Skip to content

Commit

Permalink
Permissionless token pools (#652)
Browse files Browse the repository at this point in the history
This PR is mainly focussed on the CCIP side of self serve token pools.
Some additional contracts had to be written/changed, but in review,
please focus on the CCIP & token pool side of self serve, not on the
TokenAdminRegistry.

Considerations

- Token pools are now considered hostile
  - This goes for both in- and output
  - Source side
    - Call to `sourcePool.lockOrBurn` from the onRamp
- Does not strictly have to be through a callWithExactGas call, as CCIP
is not paying for gas used here
      - Same for return data bomb
      - It returns the destPool and optional extraData
- DestPool has to be checked for length to mitigate the risk of return
data bombs through this field
- Since the onRamp is EVM2EVM, we can assume the address is an
abi.encoded(address) and therefore 32 bytes
        - ExtraData is checked like it was before
  - Dest side
    - We used to make a call to `getToken`
- Since this would be a hostile call, and require the full
callWithExactGas protection, we opted to fold this getter into the
`releaseOrMint` call. This saves a significant amount of gas, and makes
sure we only have to do a single call to the hostile contract.
    - We call `releaseOrMint` on the `pool` provided on the source side
- The pool data is considered hostile, as it came from an untrusted
source
- We need to ensure the address is valid and it contains a contract
      - The releaseOrMint call was already done through callWithExactGas

This brings the number of onchain calls to token pools to 2 per
transferred token: one on the source chain and one on the destination.
This is the absolute minimum possible.


Reviews for this PR should focus on

- Hostile token pool interactions
  - DoS
  - Unusual reverts
  - Gas bombs
  - Return data bombs
- The changes to CCIP and their impact on the security and availability
of the protocol

Please do not focus on

- The TokenAdminRegistry, except for things that would impact the
changed CCIP design


# TODO

Onchain:
- Add the tag to the return data and check for it (next PR)
- Use default price for tokens that don't have a config (next PR)

Offchain
- Add 1.4 lane logic to integration test?

---------

Co-authored-by: Anindita Ghosh <[email protected]>
  • Loading branch information
RensR and AnieeG authored Apr 12, 2024
1 parent e48517b commit 33b4251
Show file tree
Hide file tree
Showing 17 changed files with 3,719 additions and 396 deletions.
4 changes: 4 additions & 0 deletions core/services/ocr2/plugins/ccip/abihelpers/abi_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ func EncodeAbiStruct[T AbiDefined](decoded T) ([]byte, error) {
return utils.ABIEncode(decoded.AbiString(), decoded)
}

func EncodeAddress(address common.Address) ([]byte, error) {
return utils.ABIEncode(`[{"type":"address"}]`, address)
}

func DecodeAbiStruct[T AbiDefinedValid](encoded []byte) (T, error) {
var empty T

Expand Down
9 changes: 3 additions & 6 deletions core/services/ocr2/plugins/ccip/ccipexec/ocr2.go
Original file line number Diff line number Diff line change
Expand Up @@ -815,13 +815,10 @@ func getTokensPrices(ctx context.Context, priceRegistry ccipdata.PriceRegistryRe
}

for i, token := range tokens {
// price of a token can never be zero
// Prices can be zero, as we only need prices for tokens that are part of the aggregate rate limiter.
// TODO CCIP-1986: check against a list of tokens that DO need prices in the offRamp and error if a price is missing.
if fetchedPrices[i].Value.BitLen() == 0 {
priceRegistryAddress, err := priceRegistry.Address(ctx)
if err != nil {
return nil, fmt.Errorf("get price registry address: %w", err)
}
return nil, fmt.Errorf("price of token %s is zero (price registry=%s)", token, priceRegistryAddress)
continue
}

// price registry should not report different price for the same token
Expand Down
11 changes: 0 additions & 11 deletions core/services/ocr2/plugins/ccip/ccipexec/ocr2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1039,17 +1039,6 @@ func Test_getTokensPrices(t *testing.T) {
},
expErr: true,
},
{
name: "zero price should lead to an error",
feeTokens: []cciptypes.Address{tk1, tk2},
tokens: []cciptypes.Address{tk3},
retPrices: []cciptypes.TokenPriceUpdate{
{TokenPrice: cciptypes.TokenPrice{Value: big.NewInt(10)}},
{TokenPrice: cciptypes.TokenPrice{Value: big.NewInt(0)}},
{TokenPrice: cciptypes.TokenPrice{Value: big.NewInt(30)}},
},
expErr: true,
},
{
name: "contract returns less prices than requested",
feeTokens: []cciptypes.Address{tk1, tk2},
Expand Down
Loading

0 comments on commit 33b4251

Please sign in to comment.