Permissionless token pools #652

RensR · 2024-03-28T10:09:41Z

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?

# Conflicts: # contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol # contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol # contracts/src/v0.8/ccip/pools/TokenPool.sol # contracts/src/v0.8/ccip/test/StructFactory.sol # contracts/src/v0.8/ccip/test/applications/ImmutableExample.t.sol # contracts/src/v0.8/ccip/test/e2e/End2End.t.sol # contracts/src/v0.8/ccip/test/offRamp/EVM2EVMOffRamp.t.sol # contracts/src/v0.8/ccip/test/offRamp/EVM2EVMOffRampSetup.t.sol # contracts/src/v0.8/ccip/test/onRamp/EVM2EVMOnRamp.t.sol # contracts/src/v0.8/ccip/test/pools/BurnMintTokenPool.t.sol # contracts/src/v0.8/ccip/test/pools/LockReleaseTokenPool.t.sol # contracts/src/v0.8/ccip/test/pools/USDCTokenPool.t.sol

github-actions · 2024-04-10T08:51:37Z

LCOV of commit `8721682` during Solidity Foundry #3558

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

github-actions · 2024-04-10T08:53:14Z

LCOV of commit `61fe9f4` during Solidity Foundry #3559

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

github-actions · 2024-04-10T08:53:26Z

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

github-actions · 2024-04-10T09:10:04Z

LCOV of commit `eb57271` during Solidity Foundry #3561

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

elatoskinas · 2024-04-10T11:45:20Z

contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol

-
-      newMessage.sourceTokenData[i] = tokenData;
+      // Since this is an EVM2EVM message, the pool address should be exactly 32 bytes
+      if (poolReturnData.destPoolAddress.length != 32) revert InvalidAddress(poolReturnData.destPoolAddress);


question: do we need this validation here - since we already verify it in the offramp?

I'm thinking removing this validation would contribute 1 step further to breaking away from EVM2EVM specific logic

Any validation prior to sending is a win. If this is wrong, and the tx goes through, funds will be stuck in limbo due to the failure on the offRamp. Technically the sending side rate limits also offer no security value, but they are there to make sure people don't get surprised on the destination.

re breaking away: as long as this is still an EVM2EVM ramp, this check should be here. When we go for EVM2Any this can be revisited.

RayXpub · 2024-04-10T12:46:36Z

contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol

+
+      // We determined that the pool address is a valid EVM address, but that does not mean the code at this
+      // address is actually a (compatible) pool contract. If there is no contract it could not possibly be a pool.
+      if (pool.code.length == 0) {


question: isn't this check redundant with the one performed in _callWithExactGasSafeReturnData?

ccip/contracts/src/v0.8/shared/call/CallWithExactGas.sol

Lines 87 to 90 in eb57271

if iszero(extcodesize(target)) {

mstore(0x0, NO_CONTRACT_SIG)

revert(0x0, 0x4)

}

Technically no, since it would throw a NO_CONTRACT_SIG error there. I did it this way to throw a consistent error in the address parsing (see _validateEVMAddress errors) which we then catch in _trialExecute. We could make trialExecute also catch NO_CONTRACT_SIG though. I had considered both solutions, but had not benchmarked gas cost. Let me see if it's worth it

It saves 282 gas in the happy path, while the revert case costs ~3k more

core/services/ocr2/plugins/ccip/ccipexec/ocr2.go

core/services/ocr2/plugins/ccip/internal/ccipdata/v1_5_0/onramp.go

RayXpub · 2024-04-11T06:13:30Z

contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol

-      destTokenAmounts[i].token = address(pool.getToken());
-      destTokenAmounts[i].amount = sourceTokenAmount;
+      // If the call was successful, the returnData should be the local token address.
+      destTokenAmounts[i].token = _validateEVMAddress(returnData);
    }
    _rateLimitValue(destTokenAmounts, IPriceRegistry(s_dynamicConfig.priceRegistry));


question: since pools are now considered hostile, is there any risk of performing this state update after the releaseOrMint calls on the pools?

Given the very protected external call, the validation of its contents and the reentrancy protection I think we're good here. Please let me know if you have any specific action that doesn't look safe!

elatoskinas · 2024-04-11T08:12:05Z

contracts/src/v0.8/ccip/interfaces/pools/IPool.sol

@@ -6,41 +6,55 @@ import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/tok
 // Shared public interface for multiple pool types.
 // Each pool type handles a different child token model (lock/unlock, mint/burn.)
 interface IPool {
+  struct SourceTokenData {
+    bytes sourcePoolAddress;
+    bytes destPoolAddress;


suggestion: what do you think of the alternative option to pass the destToken instead of the pool? This way we can validate more strictly by querying getPool(destToken) via the registry on the dest chain - and we can always be sure that the pool represents the token correctly

In this case the mapping on the src chain would have to change to token => chainSelector => token address (bytes)

Adds an additional 5.8k gas for a single msg token and increases the size of the contract. Probably not worth it

github-actions · 2024-04-11T10:57:41Z

LCOV of commit `c899782` during Solidity Foundry #3593

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

github-actions · 2024-04-11T11:03:02Z

LCOV of commit `f51b7ae` during Solidity Foundry #3594

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

github-actions · 2024-04-11T12:27:37Z

LCOV of commit `52d559a` during Solidity Foundry #3598

Summary coverage rate:
  lines......: 98.6% (959 of 973 lines)
  functions..: 95.8% (206 of 215 functions)
  branches...: 90.4% (367 of 406 branches)

Files changed coverage rate: n/a

matYang · 2024-04-12T04:45:51Z

contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol

-
-  /// @notice Get all configured destination tokens
-  /// @return destTokens Array of configured destination tokens
-  function getDestinationTokens() external view returns (IERC20[] memory destTokens) {


@RensR Justin's PR gets all tokens supported by an offramp and filters out ones missing price getter config in job spec, so it is still requiring a trusted source for a list of supported tokens

matYang · 2024-04-12T05:21:33Z

contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol

+          sourcePoolAddress: abi.encode(sourcePool),
+          destPoolAddress: poolReturnData.destPoolAddress,
+          extraData: poolReturnData.destPoolData
+        })


while this is readable, I think this encoding adds ~224 bytes of extra calldata per token, that can add up. If no way to slim it down, we should account for it in tokenTransferBytesOverhead in billing calc.

Won't block on this, oK to mark it as todo and address in followup PR.

matYang · 2024-04-12T05:37:31Z

contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol

-    }
+  /// @inheritdoc IEVM2AnyOnRampClient
+  function getSupportedTokens(uint64 /*destChainSelector*/ ) external view returns (address[] memory) {
+    return ITokenAdminRegistry(s_dynamicConfig.tokenAdminRegistry).getAllConfiguredTokens();


There's a spike ticket for this in the current sprint 👍🏼

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]>

RensR added 10 commits March 4, 2024 11:45

rm tokens from ramps

b111359

add remote pool config to pool

60b19b0

token setup

5ad0364

fix pool return values & parsing

0c2b602

Merge branch 'ccip-develop' into permissionless-token-pools-demo

1657c8c

add mapping for token -> pool in token setup

8e0fe16

rm pools and tokens from offRamp unit

9fea02d

fix offramp setting TokenDataPayload

123638a

fix usdc & lockRelease pool

a84800a

Merge branch 'ccip-develop' into permissionless-token-pools-demo

fbda5e7

RensR added the do not merge Do not merge at this time label Mar 28, 2024

RensR self-assigned this Mar 28, 2024

RensR temporarily deployed to sdlc March 28, 2024 10:09 — with GitHub Actions Inactive

RensR added 2 commits April 2, 2024 11:03

Merge branch 'ccip-develop' into permissionless-token-pools-demo

208c889

rm s_destPools

62d8b62

RensR temporarily deployed to sdlc April 2, 2024 11:26 — with GitHub Actions Inactive

RensR added 2 commits April 2, 2024 13:53

improve MaybeRevertingBurnMintTokenPool

3325c86

fix event generation

7b0a7f7

RensR temporarily deployed to sdlc April 2, 2024 12:59 — with GitHub Actions Inactive

RensR temporarily deployed to sdlc April 2, 2024 15:22 — with GitHub Actions Inactive

fix tests & solhint

1c15e67

RensR temporarily deployed to sdlc April 3, 2024 10:59 — with GitHub Actions Inactive

smartcontractkit deleted a comment from github-actions bot Apr 3, 2024

RensR added 2 commits April 3, 2024 13:06

Update rebalancer.gas-snapshot

4ba8886

refactor with more explicit arguments

6cd835a

RensR temporarily deployed to sdlc April 3, 2024 13:53 — with GitHub Actions Inactive

add comments

8721682

RensR temporarily deployed to sdlc April 10, 2024 08:46 — with GitHub Actions Inactive

RensR temporarily deployed to sdlc April 10, 2024 08:48 — with GitHub Actions Inactive

rm old events/errors

eb57271

RensR force-pushed the permissionless-token-pools-demo branch from 61fe9f4 to eb57271 Compare April 10, 2024 09:04

RensR temporarily deployed to sdlc April 10, 2024 09:05 — with GitHub Actions Inactive

elatoskinas reviewed Apr 10, 2024

View reviewed changes

RayXpub reviewed Apr 10, 2024

View reviewed changes

jarnaud reviewed Apr 10, 2024

View reviewed changes

core/services/ocr2/plugins/ccip/ccipexec/ocr2.go Show resolved Hide resolved

mateusz-sekara reviewed Apr 10, 2024

View reviewed changes

core/services/ocr2/plugins/ccip/internal/ccipdata/v1_5_0/onramp.go Outdated Show resolved Hide resolved

RayXpub reviewed Apr 11, 2024

View reviewed changes

elatoskinas reviewed Apr 11, 2024

View reviewed changes

offchain cleanup

6bb2e73

RensR temporarily deployed to sdlc April 11, 2024 10:53 — with GitHub Actions Inactive

RensR force-pushed the permissionless-token-pools-demo branch from c899782 to f51b7ae Compare April 11, 2024 10:58

RensR temporarily deployed to sdlc April 11, 2024 10:58 — with GitHub Actions Inactive

Merge branch 'ccip-develop' into permissionless-token-pools-demo

52d559a

RensR force-pushed the permissionless-token-pools-demo branch from f51b7ae to 52d559a Compare April 11, 2024 12:23

RensR temporarily deployed to sdlc April 11, 2024 12:23 — with GitHub Actions Inactive

matYang approved these changes Apr 12, 2024

View reviewed changes

RensR merged commit 6e83f8a into ccip-develop Apr 12, 2024
79 checks passed

RensR deleted the permissionless-token-pools-demo branch April 12, 2024 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissionless token pools #652

Permissionless token pools #652

RensR commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

elatoskinas Apr 10, 2024

RensR Apr 10, 2024

RayXpub Apr 10, 2024 •

edited

Loading

RensR Apr 10, 2024

RensR Apr 10, 2024 •

edited

Loading

RayXpub Apr 11, 2024

RensR Apr 11, 2024

elatoskinas Apr 11, 2024

RensR Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

matYang Apr 12, 2024 •

edited

Loading

matYang Apr 12, 2024

matYang Apr 12, 2024 •

edited

Loading

RensR Apr 12, 2024

	if iszero(extcodesize(target)) {
	mstore(0x0, NO_CONTRACT_SIG)
	revert(0x0, 0x4)
	}

Permissionless token pools #652

Permissionless token pools #652

Conversation

RensR commented Mar 28, 2024 • edited Loading

TODO

github-actions bot commented Apr 10, 2024

LCOV of commit 8721682 during Solidity Foundry #3558

github-actions bot commented Apr 10, 2024

LCOV of commit 61fe9f4 during Solidity Foundry #3559

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

LCOV of commit eb57271 during Solidity Foundry #3561

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RayXpub Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RensR Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2024

LCOV of commit c899782 during Solidity Foundry #3593

github-actions bot commented Apr 11, 2024

LCOV of commit f51b7ae during Solidity Foundry #3594

github-actions bot commented Apr 11, 2024

LCOV of commit 52d559a during Solidity Foundry #3598

matYang Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matYang Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RensR commented Mar 28, 2024 •

edited

Loading

LCOV of commit `8721682` during Solidity Foundry #3558

LCOV of commit `61fe9f4` during Solidity Foundry #3559

LCOV of commit `eb57271` during Solidity Foundry #3561

RayXpub Apr 10, 2024 •

edited

Loading

RensR Apr 10, 2024 •

edited

Loading

LCOV of commit `c899782` during Solidity Foundry #3593

LCOV of commit `f51b7ae` during Solidity Foundry #3594

LCOV of commit `52d559a` during Solidity Foundry #3598

matYang Apr 12, 2024 •

edited

Loading

matYang Apr 12, 2024 •

edited

Loading