-
Notifications
You must be signed in to change notification settings - Fork 54
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Family-agnostic ramps - refactor fee and family-specific logic to Pri…
…ceRegistry (#1181) ## Motivation Lifts out family-specific fee & validation logic to the PriceRegistry - isolating fees calculations to the PriceRegistry state. The refactor converts the OnRamp to be fully dest-chain family agnostic - future additions of chain families would typically require only changing the PriceRegistry. ## Solution * Move the `DestChainConfig` to the PriceRegistry, and completely remove dest chain configs from the OnRamp. Since most of the dynamic config is specific to fee & chain logic (with the exception of `maxDataBytes` / `maxNumberOfTokens`), it is optimal to store the configs in a single contract * Moves all fee token configs to the PriceRegistry * Removes duplicate `_validateMessage`. Since the Router always calls `getFee` before `forwardFromRouter` - we can do the validations early in `getFee` * `PriceRegistry.getValidatedFee` - performs the previous `getFee` logic * `PriceRegistry.getValidatedRampMessageParams` - performs the chain-specific logic of `forwardFromRouter` - validating pool return data, computing message fees and determining out-of-order exec ![alt-chain-family-handling-design-Page-2 drawio (1)](https://github.com/user-attachments/assets/79603dde-6531-4d17-bf8b-ae80eaf0dbfc) ### Contract Sizes | Contract | Prev Size (kB) | Prev Margin (kB) | New Size (kB) | New Margin (kB) | |---------------------------------|------------------------------|---------------------------|---------------------------|---------------------------| | EVM2EVMMultiOnRamp | 22.6 | 1.976 | 9.944 | 14.632 | | PriceRegistry | 7.46 | 17.116 | 18.439 | 6.137 | | **Total** | 30.06 | | 28.383 | | ### Gas costs (e2e) | Function | Prev min-max | Prev avg | New min-max | New avg | |--------------|--------------|---------------|----------------|---------| | getFee (MultiOnRamp) | 0 - 47 501 | 20 357 | 0 - 38 863 | 16 655 | ccipSend (Router) | 287 347 - 375 060 | 341 643 | 279 728 - 367 441 | 334 024 --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> Co-authored-by: Makram <[email protected]>
- Loading branch information
1 parent
be120a2
commit ef923c3
Showing
26 changed files
with
5,173 additions
and
3,994 deletions.
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
118 changes: 118 additions & 0 deletions
118
contracts/src/v0.8/ccip/test/attacks/onRamp/MultiOnRampTokenPoolReentrancy.t.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
pragma solidity 0.8.24; | ||
|
||
import {Client} from "../../../libraries/Client.sol"; | ||
import {Internal} from "../../../libraries/Internal.sol"; | ||
import {EVM2EVMMultiOnRamp} from "../../../onRamp/EVM2EVMMultiOnRamp.sol"; | ||
import {TokenPool} from "../../../pools/TokenPool.sol"; | ||
import {EVM2EVMMultiOnRampSetup} from "../../onRamp/EVM2EVMMultiOnRampSetup.t.sol"; | ||
import {FacadeClient} from "./FacadeClient.sol"; | ||
import {ReentrantMaliciousTokenPool} from "./ReentrantMaliciousTokenPool.sol"; | ||
|
||
import {IERC20} from "../../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; | ||
|
||
import {console} from "forge-std/console.sol"; | ||
|
||
/// @title MultiOnRampTokenPoolReentrancy | ||
/// Attempts to perform a reentrancy exploit on Onramp with a malicious TokenPool | ||
contract MultiOnRampTokenPoolReentrancy is EVM2EVMMultiOnRampSetup { | ||
FacadeClient internal s_facadeClient; | ||
ReentrantMaliciousTokenPool internal s_maliciousTokenPool; | ||
IERC20 internal s_sourceToken; | ||
IERC20 internal s_feeToken; | ||
address internal immutable i_receiver = makeAddr("receiver"); | ||
|
||
function setUp() public virtual override { | ||
EVM2EVMMultiOnRampSetup.setUp(); | ||
|
||
s_sourceToken = IERC20(s_sourceTokens[0]); | ||
s_feeToken = IERC20(s_sourceTokens[0]); | ||
|
||
s_facadeClient = | ||
new FacadeClient(address(s_sourceRouter), DEST_CHAIN_SELECTOR, s_sourceToken, s_feeToken, i_receiver); | ||
|
||
s_maliciousTokenPool = new ReentrantMaliciousTokenPool( | ||
address(s_facadeClient), s_sourceToken, address(s_mockRMN), address(s_sourceRouter) | ||
); | ||
|
||
TokenPool.ChainUpdate[] memory chainUpdates = new TokenPool.ChainUpdate[](1); | ||
chainUpdates[0] = TokenPool.ChainUpdate({ | ||
remoteChainSelector: DEST_CHAIN_SELECTOR, | ||
remotePoolAddress: abi.encode(s_destPoolBySourceToken[s_sourceTokens[0]]), | ||
remoteTokenAddress: abi.encode(s_destTokens[0]), | ||
allowed: true, | ||
outboundRateLimiterConfig: getOutboundRateLimiterConfig(), | ||
inboundRateLimiterConfig: getInboundRateLimiterConfig() | ||
}); | ||
s_maliciousTokenPool.applyChainUpdates(chainUpdates); | ||
s_sourcePoolByToken[address(s_sourceToken)] = address(s_maliciousTokenPool); | ||
|
||
Internal.PoolUpdate[] memory removes = new Internal.PoolUpdate[](1); | ||
removes[0].token = address(s_sourceToken); | ||
removes[0].pool = address(s_sourcePoolByToken[address(s_sourceToken)]); | ||
Internal.PoolUpdate[] memory adds = new Internal.PoolUpdate[](1); | ||
adds[0].token = address(s_sourceToken); | ||
adds[0].pool = address(s_maliciousTokenPool); | ||
|
||
s_tokenAdminRegistry.setPool(address(s_sourceToken), address(s_maliciousTokenPool)); | ||
|
||
s_sourceToken.transfer(address(s_facadeClient), 1e18); | ||
s_feeToken.transfer(address(s_facadeClient), 1e18); | ||
} | ||
|
||
/// @dev This test was used to showcase a reentrancy exploit on OnRamp with malicious TokenPool. | ||
/// How it worked: OnRamp used to construct EVM2Any messages after calling TokenPool's lockOrBurn. | ||
/// This allowed the malicious TokenPool to break message sequencing expectations as follows: | ||
/// Any user -> Facade -> 1st call to ccipSend -> pool’s lockOrBurn —> | ||
/// (reenter)-> Facade -> 2nd call to ccipSend | ||
/// In this case, Facade's second call would produce an EVM2Any msg with a lower sequence number. | ||
/// The issue was fixed by moving state updates and event construction to before TokenPool calls. | ||
/// This test is kept to verify message sequence expectations are not broken. | ||
function test_OnRampTokenPoolReentrancy_Success() public { | ||
uint256 amount = 1; | ||
|
||
Client.EVMTokenAmount[] memory tokenAmounts = new Client.EVMTokenAmount[](1); | ||
tokenAmounts[0].token = address(s_sourceToken); | ||
tokenAmounts[0].amount = amount; | ||
|
||
Client.EVM2AnyMessage memory message1 = Client.EVM2AnyMessage({ | ||
receiver: abi.encode(i_receiver), | ||
data: abi.encodePacked(uint256(1)), // message 1 contains data 1 | ||
tokenAmounts: tokenAmounts, | ||
extraArgs: Client._argsToBytes(Client.EVMExtraArgsV1({gasLimit: 200_000})), | ||
feeToken: address(s_feeToken) | ||
}); | ||
|
||
Client.EVM2AnyMessage memory message2 = Client.EVM2AnyMessage({ | ||
receiver: abi.encode(i_receiver), | ||
data: abi.encodePacked(uint256(2)), // message 2 contains data 2 | ||
tokenAmounts: tokenAmounts, | ||
extraArgs: Client._argsToBytes(Client.EVMExtraArgsV1({gasLimit: 200_000})), | ||
feeToken: address(s_feeToken) | ||
}); | ||
|
||
uint256 expectedFee = s_sourceRouter.getFee(DEST_CHAIN_SELECTOR, message1); | ||
assertGt(expectedFee, 0); | ||
|
||
// Outcome of a successful exploit: | ||
// Message 1 event from OnRamp contains sequence/nonce 2, message 2 contains sequence/nonce 1 | ||
// Internal.EVM2EVMMessage memory msgEvent1 = _messageToEvent(message1, 2, 2, expectedFee, address(s_facadeClient)); | ||
// Internal.EVM2EVMMessage memory msgEvent2 = _messageToEvent(message2, 1, 1, expectedFee, address(s_facadeClient)); | ||
|
||
// vm.expectEmit(); | ||
// emit CCIPSendRequested(msgEvent2); | ||
// vm.expectEmit(); | ||
// emit CCIPSendRequested(msgEvent1); | ||
|
||
// After issue is fixed, sequence now increments as expected | ||
Internal.EVM2AnyRampMessage memory msgEvent1 = _messageToEvent(message1, 1, 1, expectedFee, address(s_facadeClient)); | ||
Internal.EVM2AnyRampMessage memory msgEvent2 = _messageToEvent(message2, 2, 2, expectedFee, address(s_facadeClient)); | ||
|
||
vm.expectEmit(); | ||
emit EVM2EVMMultiOnRamp.CCIPSendRequested(DEST_CHAIN_SELECTOR, msgEvent2); | ||
vm.expectEmit(); | ||
emit EVM2EVMMultiOnRamp.CCIPSendRequested(DEST_CHAIN_SELECTOR, msgEvent1); | ||
|
||
s_facadeClient.send(amount); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
72 changes: 72 additions & 0 deletions
72
contracts/src/v0.8/ccip/test/helpers/PriceRegistryHelper.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
pragma solidity 0.8.24; | ||
|
||
import {PriceRegistry} from "../../PriceRegistry.sol"; | ||
import {Client} from "../../libraries/Client.sol"; | ||
|
||
contract PriceRegistryHelper is PriceRegistry { | ||
constructor( | ||
StaticConfig memory staticConfig, | ||
address[] memory priceUpdaters, | ||
address[] memory feeTokens, | ||
TokenPriceFeedUpdate[] memory tokenPriceFeeds, | ||
TokenTransferFeeConfigArgs[] memory tokenTransferFeeConfigArgs, | ||
PremiumMultiplierWeiPerEthArgs[] memory premiumMultiplierWeiPerEthArgs, | ||
DestChainConfigArgs[] memory destChainConfigArgs | ||
) | ||
PriceRegistry( | ||
staticConfig, | ||
priceUpdaters, | ||
feeTokens, | ||
tokenPriceFeeds, | ||
tokenTransferFeeConfigArgs, | ||
premiumMultiplierWeiPerEthArgs, | ||
destChainConfigArgs | ||
) | ||
{} | ||
|
||
function getDataAvailabilityCost( | ||
uint64 destChainSelector, | ||
uint112 dataAvailabilityGasPrice, | ||
uint256 messageDataLength, | ||
uint256 numberOfTokens, | ||
uint32 tokenTransferBytesOverhead | ||
) external view returns (uint256) { | ||
return _getDataAvailabilityCost( | ||
s_destChainConfigs[destChainSelector], | ||
dataAvailabilityGasPrice, | ||
messageDataLength, | ||
numberOfTokens, | ||
tokenTransferBytesOverhead | ||
); | ||
} | ||
|
||
function getTokenTransferCost( | ||
uint64 destChainSelector, | ||
address feeToken, | ||
uint224 feeTokenPrice, | ||
Client.EVMTokenAmount[] calldata tokenAmounts | ||
) external view returns (uint256, uint32, uint32) { | ||
return _getTokenTransferCost( | ||
s_destChainConfigs[destChainSelector], destChainSelector, feeToken, feeTokenPrice, tokenAmounts | ||
); | ||
} | ||
|
||
function parseEVMExtraArgsFromBytes( | ||
bytes calldata extraArgs, | ||
uint64 destChainSelector | ||
) external view returns (Client.EVMExtraArgsV2 memory) { | ||
return _parseEVMExtraArgsFromBytes(extraArgs, s_destChainConfigs[destChainSelector]); | ||
} | ||
|
||
function parseEVMExtraArgsFromBytes( | ||
bytes calldata extraArgs, | ||
DestChainConfig memory destChainConfig | ||
) external pure returns (Client.EVMExtraArgsV2 memory) { | ||
return _parseEVMExtraArgsFromBytes(extraArgs, destChainConfig); | ||
} | ||
|
||
function validateDestFamilyAddress(bytes4 chainFamilySelector, bytes memory destAddress) external pure { | ||
_validateDestFamilyAddress(chainFamilySelector, destAddress); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.