Skip to content

Commit

Permalink
undo transferFrom, assert balance in offRamp (#1273)
Browse files Browse the repository at this point in the history
## Motivation
It is significantly cheaper to assert balances than do the transferFrom

## Solution
Undo recent approve changes, add balance assertions


had to undo some of the multiOfframp calldata changes to make it fit

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
  • Loading branch information
2 people authored and asoliman92 committed Aug 28, 2024
1 parent 54b8aac commit be239b8
Show file tree
Hide file tree
Showing 41 changed files with 664 additions and 402 deletions.
430 changes: 219 additions & 211 deletions contracts/gas-snapshots/ccip.gas-snapshot

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions contracts/gas-snapshots/liquiditymanager.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ LiquidityManager_addLiquidity:test_addLiquiditySuccess() (gas: 279154)
LiquidityManager_rebalanceLiquidity:test_InsufficientLiquidityReverts() (gas: 206745)
LiquidityManager_rebalanceLiquidity:test_InvalidRemoteChainReverts() (gas: 192319)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess() (gas: 9141768)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess_AlreadyFinalized() (gas: 8981838)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 8977044)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 8904842)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess_AlreadyFinalized() (gas: 8960797)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 8956003)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 8883801)
LiquidityManager_rebalanceLiquidity:test_rebalanceLiquiditySuccess() (gas: 382897)
LiquidityManager_receive:test_receive_success() (gas: 21182)
LiquidityManager_removeLiquidity:test_InsufficientLiquidityReverts() (gas: 184869)
Expand All @@ -19,7 +19,7 @@ LiquidityManager_setFinanceRole:test_OnlyOwnerReverts() (gas: 10987)
LiquidityManager_setFinanceRole:test_setFinanceRoleSuccess() (gas: 21836)
LiquidityManager_setLocalLiquidityContainer:test_OnlyOwnerReverts() (gas: 11052)
LiquidityManager_setLocalLiquidityContainer:test_ReverstWhen_CalledWithTheZeroAddress() (gas: 10643)
LiquidityManager_setLocalLiquidityContainer:test_setLocalLiquidityContainerSuccess() (gas: 3519863)
LiquidityManager_setLocalLiquidityContainer:test_setLocalLiquidityContainerSuccess() (gas: 3498806)
LiquidityManager_setMinimumLiquidity:test_OnlyOwnerReverts() (gas: 10925)
LiquidityManager_setMinimumLiquidity:test_setMinimumLiquiditySuccess() (gas: 36389)
LiquidityManager_withdrawERC20:test_withdrawERC20Reverts() (gas: 180359)
Expand Down
2 changes: 1 addition & 1 deletion contracts/scripts/native_solc_compile_all_ccip
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ echo " └───────────────────────

SOLC_VERSION="0.8.24"
OPTIMIZE_RUNS=26000
OPTIMIZE_RUNS_OFFRAMP=18000
OPTIMIZE_RUNS_OFFRAMP=22000
OPTIMIZE_RUNS_ONRAMP=4100
OPTIMIZE_RUNS_MULTI_OFFRAMP=2000

Expand Down
2 changes: 2 additions & 0 deletions contracts/src/v0.8/ccip/interfaces/IPool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ interface IPoolV1 is IERC165 {
/// @param releaseOrMintIn All data required to release or mint tokens.
/// @return releaseOrMintOut The amount of tokens released or minted on the local chain, denominated
/// in the local token's decimals.
/// @dev The offramp asserts that the balanceOf of the receiver has been incremented by exactly the number
/// of tokens that is returned in ReleaseOrMintOutV1.destinationAmount. If the amounts do not match, the tx reverts.
function releaseOrMint(Pool.ReleaseOrMintInV1 calldata releaseOrMintIn)
external
returns (Pool.ReleaseOrMintOutV1 memory);
Expand Down
2 changes: 2 additions & 0 deletions contracts/src/v0.8/ccip/libraries/Internal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ library Internal {
// malicious contracts from returning large amounts of data and causing
// repeated out-of-gas scenarios.
uint16 internal constant MAX_RET_BYTES = 4 + 4 * 32;
/// @dev The expected number of bytes returned by the balanceOf function.
uint256 internal constant MAX_BALANCE_OF_RET_BYTES = 32;

/// @notice A collection of token price and gas price updates.
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
Expand Down
3 changes: 1 addition & 2 deletions contracts/src/v0.8/ccip/libraries/Pool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ library Pool {
struct ReleaseOrMintInV1 {
bytes originalSender; // The original sender of the tx on the source chain
uint64 remoteChainSelector; // ─╮ The chain ID of the source chain
address receiver; // ───────────╯ The recipient of the tokens on the destination chain. This is *NOT* the address to
// send the tokens to, but the address that will receive the tokens via the offRamp.
address receiver; // ───────────╯ The recipient of the tokens on the destination chain.
uint256 amount; // The amount of tokens to release or mint, denominated in the source token's decimals
address localToken; // The address on this chain of the token to release or mint
/// @dev WARNING: sourcePoolAddress should be checked prior to any processing of funds. Make sure it matches the
Expand Down
63 changes: 46 additions & 17 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
error CanOnlySelfCall();
error ReceiverError(bytes err);
error TokenHandlingError(bytes err);
error ReleaseOrMintBalanceMismatch(uint256 amountReleased, uint256 balancePre, uint256 balancePost);
error EmptyReport();
error CursedByRMN(uint64 sourceChainSelector);
error NotACompatiblePool(address notPool);
Expand Down Expand Up @@ -497,7 +498,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
/// @dev We use ERC-165 to check for the ccipReceive interface to permit sending tokens to contracts
/// (for example smart contract wallets) without an associated message.
function executeSingleMessage(
Internal.Any2EVMRampMessage calldata message,
Internal.Any2EVMRampMessage memory message,
bytes[] calldata offchainTokenData
) external {
if (msg.sender != address(this)) revert CanOnlySelfCall();
Expand Down Expand Up @@ -807,11 +808,11 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
/// @param offchainTokenData Data fetched offchain by the DON.
/// @return destTokenAmount local token address with amount
function _releaseOrMintSingleToken(
Internal.RampTokenAmount calldata sourceTokenAmount,
bytes calldata originalSender,
Internal.RampTokenAmount memory sourceTokenAmount,
bytes memory originalSender,
address receiver,
uint64 sourceChainSelector,
bytes calldata offchainTokenData
bytes memory offchainTokenData
) internal returns (Client.EVMTokenAmount memory destTokenAmount) {
// We need to safely decode the token address from the sourceTokenData, as it could be wrong,
// in which case it doesn't have to be a valid EVM address.
Expand All @@ -826,12 +827,17 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
revert NotACompatiblePool(localPoolAddress);
}

// We retrieve the local token balance of the receiver before the pool call.
(uint256 balancePre, uint256 gasLeft) =
_getBalanceOfReceiver(receiver, localToken, s_dynamicConfig.maxPoolReleaseOrMintGas);

// We determined that the pool address is a valid EVM address, but that does not mean the code at this
// address is a (compatible) pool contract. _callWithExactGasSafeReturnData will check if the location
// contains a contract. If it doesn't it reverts with a known error, which we catch gracefully.
// We call the pool with exact gas to increase resistance against malicious tokens or token pools.
// We protects against return data bombs by capping the return data size at MAX_RET_BYTES.
(bool success, bytes memory returnData,) = CallWithExactGas._callWithExactGasSafeReturnData(
(bool success, bytes memory returnData, uint256 gasUsedReleaseOrMint) = CallWithExactGas
._callWithExactGasSafeReturnData(
abi.encodeCall(
IPoolV1.releaseOrMint,
Pool.ReleaseOrMintInV1({
Expand All @@ -846,7 +852,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
})
),
localPoolAddress,
s_dynamicConfig.maxPoolReleaseOrMintGas,
gasLeft,
Internal.GAS_FOR_CALL_EXACT_CHECK,
Internal.MAX_RET_BYTES
);
Expand All @@ -858,21 +864,44 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
if (returnData.length != Pool.CCIP_POOL_V1_RET_BYTES) {
revert InvalidDataLength(Pool.CCIP_POOL_V1_RET_BYTES, returnData.length);
}

uint256 localAmount = abi.decode(returnData, (uint256));
// Since token pools send the tokens to the msg.sender, which is this offRamp, we need to
// transfer them to the final receiver. We use the _callWithExactGasSafeReturnData function because
// the token contracts are not considered trusted.
(success, returnData,) = CallWithExactGas._callWithExactGasSafeReturnData(
abi.encodeCall(IERC20.transferFrom, (localPoolAddress, receiver, localAmount)),
localToken,
s_dynamicConfig.maxTokenTransferGas,
// We don't need to do balance checks if the pool is the receiver, as they would always fail in the case
// of a lockRelease pool.
if (receiver != localPoolAddress) {
(uint256 balancePost,) = _getBalanceOfReceiver(receiver, localToken, gasLeft - gasUsedReleaseOrMint);

// First we check if the subtraction would result in an underflow to ensure we revert with a clear error
if (balancePost < balancePre || balancePost - balancePre != localAmount) {
revert ReleaseOrMintBalanceMismatch(localAmount, balancePre, balancePost);
}
}

return Client.EVMTokenAmount({token: localToken, amount: localAmount});
}

function _getBalanceOfReceiver(
address receiver,
address token,
uint256 gasLimit
) internal returns (uint256 balance, uint256 gasLeft) {
(bool success, bytes memory returnData, uint256 gasUsed) = CallWithExactGas._callWithExactGasSafeReturnData(
abi.encodeCall(IERC20.balanceOf, (receiver)),
token,
gasLimit,
Internal.GAS_FOR_CALL_EXACT_CHECK,
Internal.MAX_RET_BYTES
);

if (!success) revert TokenHandlingError(returnData);

return Client.EVMTokenAmount({token: localToken, amount: localAmount});
// If the call was successful, the returnData should contain only the balance.
if (returnData.length != Internal.MAX_BALANCE_OF_RET_BYTES) {
revert InvalidDataLength(Internal.MAX_BALANCE_OF_RET_BYTES, returnData.length);
}

// Return the decoded balance, which cannot fail as we checked the length, and the gas that is left
// after this call.
return (abi.decode(returnData, (uint256)), gasLimit - gasUsed);
}

/// @notice Uses pools to release or mint a number of different tokens to a receiver address.
Expand All @@ -886,8 +915,8 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
/// any non-rate limiting errors that may occur. If we encounter a rate limiting related error
/// we bubble it up. If we encounter a non-rate limiting error we wrap it in a TokenHandlingError.
function _releaseOrMintTokens(
Internal.RampTokenAmount[] calldata sourceTokenAmounts,
bytes calldata originalSender,
Internal.RampTokenAmount[] memory sourceTokenAmounts,
bytes memory originalSender,
address receiver,
uint64 sourceChainSelector,
bytes[] calldata offchainTokenData
Expand Down
56 changes: 41 additions & 15 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
error CanOnlySelfCall();
error ReceiverError(bytes err);
error TokenHandlingError(bytes err);
error ReleaseOrMintBalanceMismatch(uint256 amountReleased, uint256 balancePre, uint256 balancePost);
error EmptyReport();
error CursedByRMN();
error InvalidMessageId();
Expand Down Expand Up @@ -459,7 +460,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
/// its execution and enforce atomicity among successful message processing and token transfer.
/// @dev We use ERC-165 to check for the ccipReceive interface to permit sending tokens to contracts
/// (for example smart contract wallets) without an associated message.
function executeSingleMessage(Internal.EVM2EVMMessage memory message, bytes[] memory offchainTokenData) external {
function executeSingleMessage(Internal.EVM2EVMMessage calldata message, bytes[] calldata offchainTokenData) external {
if (msg.sender != address(this)) revert CanOnlySelfCall();
Client.EVMTokenAmount[] memory destTokenAmounts = new Client.EVMTokenAmount[](0);
if (message.tokenAmounts.length > 0) {
Expand Down Expand Up @@ -620,6 +621,9 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
revert NotACompatiblePool(localPoolAddress);
}

// We retrieve the local token balance of the receiver before the pool call.
(uint256 balancePre, uint256 gasLeft) = _getBalanceOfReceiver(receiver, localToken, sourceTokenData.destGasAmount);

// We determined that the pool address is a valid EVM address, but that does not mean the code at this
// address is a (compatible) pool contract. _callWithExactGasSafeReturnData will check if the location
// contains a contract. If it doesn't it reverts with a known error, which we catch gracefully.
Expand All @@ -641,33 +645,55 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
})
),
localPoolAddress,
sourceTokenData.destGasAmount,
gasLeft,
Internal.GAS_FOR_CALL_EXACT_CHECK,
Internal.MAX_RET_BYTES
);

// wrap and rethrow the error so we can catch it lower in the stack
if (!success) revert TokenHandlingError(returnData);

// If the call was successful, the returnData should contain only the local token amount.
if (returnData.length != Pool.CCIP_POOL_V1_RET_BYTES) {
revert InvalidDataLength(Pool.CCIP_POOL_V1_RET_BYTES, returnData.length);
}

uint256 localAmount = abi.decode(returnData, (uint256));
// Since token pools send the tokens to the msg.sender, which is this offRamp, we need to
// transfer them to the final receiver. We use the _callWithExactGasSafeReturnData function because
// the token contracts are not considered trusted.
(success, returnData,) = CallWithExactGas._callWithExactGasSafeReturnData(
abi.encodeCall(IERC20.transferFrom, (localPoolAddress, receiver, localAmount)),
localToken,
sourceTokenData.destGasAmount - gasUsedReleaseOrMint,
// We don't need to do balance checks if the pool is the receiver, as they would always fail in the case
// of a lockRelease pool.
if (receiver != localPoolAddress) {
(uint256 balancePost,) = _getBalanceOfReceiver(receiver, localToken, gasLeft - gasUsedReleaseOrMint);

// First we check if the subtraction would result in an underflow to ensure we revert with a clear error
if (balancePost < balancePre || balancePost - balancePre != localAmount) {
revert ReleaseOrMintBalanceMismatch(localAmount, balancePre, balancePost);
}
}

return Client.EVMTokenAmount({token: localToken, amount: localAmount});
}

function _getBalanceOfReceiver(
address receiver,
address token,
uint256 gasLimit
) internal returns (uint256 balance, uint256 gasLeft) {
(bool success, bytes memory returnData, uint256 gasUsed) = CallWithExactGas._callWithExactGasSafeReturnData(
abi.encodeCall(IERC20.balanceOf, (receiver)),
token,
gasLimit,
Internal.GAS_FOR_CALL_EXACT_CHECK,
Internal.MAX_RET_BYTES
);

if (!success) revert TokenHandlingError(returnData);

return Client.EVMTokenAmount({token: localToken, amount: localAmount});
// If the call was successful, the returnData should contain only the balance.
if (returnData.length != Internal.MAX_BALANCE_OF_RET_BYTES) {
revert InvalidDataLength(Internal.MAX_BALANCE_OF_RET_BYTES, returnData.length);
}

// Return the decoded balance, which cannot fail as we checked the length, and the gas that is left
// after this call.
return (abi.decode(returnData, (uint256)), gasLimit - gasUsed);
}

/// @notice Uses pools to release or mint a number of different tokens to a receiver address.
Expand All @@ -680,11 +706,11 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
/// any non-rate limiting errors that may occur. If we encounter a rate limiting related error
/// we bubble it up. If we encounter a non-rate limiting error we wrap it in a TokenHandlingError.
function _releaseOrMintTokens(
Client.EVMTokenAmount[] memory sourceTokenAmounts,
Client.EVMTokenAmount[] calldata sourceTokenAmounts,
bytes memory originalSender,
address receiver,
bytes[] memory encodedSourceTokenData,
bytes[] memory offchainTokenData
bytes[] calldata encodedSourceTokenData,
bytes[] calldata offchainTokenData
) internal returns (Client.EVMTokenAmount[] memory destTokenAmounts) {
// Creating a copy is more gas efficient than initializing a new array.
destTokenAmounts = sourceTokenAmounts;
Expand Down
5 changes: 2 additions & 3 deletions contracts/src/v0.8/ccip/pools/BurnMintTokenPoolAbstract.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ abstract contract BurnMintTokenPoolAbstract is TokenPool {
{
_validateReleaseOrMint(releaseOrMintIn);

// Mint to the offRamp, which forwards it to the recipient
IBurnMintERC20(address(i_token)).mint(address(this), releaseOrMintIn.amount);
IBurnMintERC20(address(i_token)).approve(msg.sender, releaseOrMintIn.amount);
// Mint to the receiver
IBurnMintERC20(address(i_token)).mint(releaseOrMintIn.receiver, releaseOrMintIn.amount);

emit Minted(msg.sender, releaseOrMintIn.receiver, releaseOrMintIn.amount);

Expand Down
3 changes: 1 addition & 2 deletions contracts/src/v0.8/ccip/pools/BurnMintTokenPoolAndProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ contract BurnMintTokenPoolAndProxy is ITypeAndVersion, LegacyPoolWrapper {
_validateReleaseOrMint(releaseOrMintIn);

if (!_hasLegacyPool()) {
// Mint to the offRamp, which forwards it to the recipient
IBurnMintERC20(address(i_token)).mint(msg.sender, releaseOrMintIn.amount);
IBurnMintERC20(address(i_token)).mint(releaseOrMintIn.receiver, releaseOrMintIn.amount);
} else {
_releaseOrMintLegacy(releaseOrMintIn);
}
Expand Down
7 changes: 5 additions & 2 deletions contracts/src/v0.8/ccip/pools/LegacyPoolWrapper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,11 @@ abstract contract LegacyPoolWrapper is TokenPool {
/// @dev Since extraData has never been used in LockRelease or MintBurn token pools, we can safely ignore it.
function _releaseOrMintLegacy(Pool.ReleaseOrMintInV1 memory releaseOrMintIn) internal {
s_previousPool.releaseOrMint(
releaseOrMintIn.originalSender, address(this), releaseOrMintIn.amount, releaseOrMintIn.remoteChainSelector, ""
releaseOrMintIn.originalSender,
releaseOrMintIn.receiver,
releaseOrMintIn.amount,
releaseOrMintIn.remoteChainSelector,
""
);
i_token.approve(msg.sender, releaseOrMintIn.amount);
}
}
4 changes: 2 additions & 2 deletions contracts/src/v0.8/ccip/pools/LockReleaseTokenPool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion
{
_validateReleaseOrMint(releaseOrMintIn);

// Release to the offRamp, which forwards it to the recipient
getToken().approve(msg.sender, releaseOrMintIn.amount);
// Release to the recipient
getToken().safeTransfer(releaseOrMintIn.receiver, releaseOrMintIn.amount);

emit Released(msg.sender, releaseOrMintIn.receiver, releaseOrMintIn.amount);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ contract LockReleaseTokenPoolAndProxy is LegacyPoolWrapper, ILiquidityContainer,
_validateReleaseOrMint(releaseOrMintIn);

if (!_hasLegacyPool()) {
// Release to the offRamp, which forwards it to the recipient
getToken().safeTransfer(msg.sender, releaseOrMintIn.amount);
// Release to the recipient
getToken().safeTransfer(releaseOrMintIn.receiver, releaseOrMintIn.amount);
} else {
_releaseOrMintLegacy(releaseOrMintIn);
}
Expand Down
Loading

0 comments on commit be239b8

Please sign in to comment.