Skip to content

Commit

Permalink
Add extra check for decimal math (#15419)
Browse files Browse the repository at this point in the history 
* improve pool tests

* add decimal & overflow checks

* add test cov

* gen wrappers

* add test

* liq man snap

* fix usdc test

* changeset

* gas golf decimal check
  • Loading branch information
@RensR
RensR authored Nov 28, 2024
1 parent 936d9fd commit 22bcd4b
Show file tree
Hide file tree
Showing 27 changed files with 459 additions and 501 deletions.
5 changes: 5 additions & 0 deletions contracts/.changeset/three-dogs-return.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@chainlink/contracts': patch
---

extra validation on decimal logic in token pools
184 changes: 93 additions & 91 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions contracts/gas-snapshots/liquiditymanager.gas-snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ LiquidityManager_addLiquidity:test_addLiquiditySuccess() (gas: 279198)
LiquidityManager_rebalanceLiquidity:test_InsufficientLiquidityReverts() (gas: 206764)
LiquidityManager_rebalanceLiquidity:test_InvalidRemoteChainReverts() (gas: 192374)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess() (gas: 9141798)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess_AlreadyFinalized() (gas: 9246772)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 9241912)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 9169653)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess_AlreadyFinalized() (gas: 9306766)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 9301906)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 9231739)
LiquidityManager_rebalanceLiquidity:test_rebalanceLiquiditySuccess() (gas: 382928)
LiquidityManager_receive:test_receive_success() (gas: 21182)
LiquidityManager_removeLiquidity:test_InsufficientLiquidityReverts() (gas: 184959)
Expand All @@ -19,7 +19,7 @@ LiquidityManager_setFinanceRole:test_OnlyOwnerReverts() (gas: 10987)
LiquidityManager_setFinanceRole:test_setFinanceRoleSuccess() (gas: 21836)
LiquidityManager_setLocalLiquidityContainer:test_OnlyOwnerReverts() (gas: 11030)
LiquidityManager_setLocalLiquidityContainer:test_ReverstWhen_CalledWithTheZeroAddress() (gas: 10621)
LiquidityManager_setLocalLiquidityContainer:test_setLocalLiquidityContainerSuccess() (gas: 3784709)
LiquidityManager_setLocalLiquidityContainer:test_setLocalLiquidityContainerSuccess() (gas: 3847203)
LiquidityManager_setMinimumLiquidity:test_OnlyOwnerReverts() (gas: 10925)
LiquidityManager_setMinimumLiquidity:test_setMinimumLiquiditySuccess() (gas: 36389)
LiquidityManager_withdrawERC20:test_withdrawERC20Reverts() (gas: 180396)
Expand Down
1 change: 0 additions & 1 deletion contracts/scripts/lcov_prune
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ exclusion_list_ccip=(
"src/v0.8/ConfirmedOwnerWithProposal.sol"
"src/v0.8/tests/MockV3Aggregator.sol"
"src/v0.8/ccip/applications/CCIPClientExample.sol"
"src/v0.8/ccip/pools/BurnWithFromMintTokenPool.sol"
)

exclusion_list_shared=(
Expand Down
39 changes: 34 additions & 5 deletions contracts/src/v0.8/ccip/pools/TokenPool.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {Pool} from "../libraries/Pool.sol";
import {RateLimiter} from "../libraries/RateLimiter.sol";

import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";
import {IERC20Metadata} from
"../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import {IERC165} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/introspection/IERC165.sol";
import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/structs/EnumerableSet.sol";

Expand Down Expand Up @@ -49,6 +51,8 @@ abstract contract TokenPool is IPoolV1, Ownable2StepMsgSender {
error PoolAlreadyAdded(uint64 remoteChainSelector, bytes remotePoolAddress);
error InvalidRemotePoolForChain(uint64 remoteChainSelector, bytes remotePoolAddress);
error InvalidRemoteChainDecimals(bytes sourcePoolData);
error OverflowDetected(uint8 remoteDecimals, uint8 localDecimals, uint256 remoteAmount);
error InvalidDecimalArgs(uint8 expected, uint8 actual);

event Locked(address indexed sender, uint256 amount);
event Burned(address indexed sender, uint256 amount);
Expand Down Expand Up @@ -119,7 +123,17 @@ abstract contract TokenPool is IPoolV1, Ownable2StepMsgSender {
if (address(token) == address(0) || router == address(0) || rmnProxy == address(0)) revert ZeroAddressNotAllowed();
i_token = token;
i_rmnProxy = rmnProxy;

try IERC20Metadata(address(token)).decimals() returns (uint8 actualTokenDecimals) {
if (localTokenDecimals != actualTokenDecimals) {
revert InvalidDecimalArgs(localTokenDecimals, actualTokenDecimals);
}
} catch {
// The decimals function doesn't exist, which is possible since it's optional in the ERC20 spec. We skip the check and
// assume the supplied token decimals are correct.
}
i_tokenDecimals = localTokenDecimals;

s_router = IRouter(router);

// Pool can be set as permissioned or permissionless at deployment time only to save hot-path gas.
Expand Down Expand Up @@ -256,18 +270,33 @@ abstract contract TokenPool is IPoolV1, Ownable2StepMsgSender {
/// @param remoteAmount The amount on the remote chain.
/// @param remoteDecimals The decimals of the token on the remote chain.
/// @return The local amount.
/// @dev This function assumes the inputs don't overflow and does no checks to avoid this. For any normal inputs, this
/// should not be a problem. The only way to overflow is when the given arguments cannot be represented in the uint256
/// type, which means the inputs are invalid.
/// @dev This function protects against overflows. If there is a transaction that hits the overflow check, it is
/// probably incorrect as that means the amount cannot be represented on this chain. If the local decimals have been
/// wrongly configured, the token issuer could redeploy the pool with the correct decimals and manually re-execute the
/// CCIP tx to fix the issue.
function _calculateLocalAmount(uint256 remoteAmount, uint8 remoteDecimals) internal view virtual returns (uint256) {
if (remoteDecimals == i_tokenDecimals) {
return remoteAmount;
}
if (remoteDecimals > i_tokenDecimals) {
uint8 decimalsDiff = remoteDecimals - i_tokenDecimals;
if (decimalsDiff > 77) {
// This is a safety check to prevent overflow in the next calculation.
revert OverflowDetected(remoteDecimals, i_tokenDecimals, remoteAmount);
}
// Solidity rounds down so there is no risk of minting more tokens than the remote chain sent.
return remoteAmount / (10 ** (remoteDecimals - i_tokenDecimals));
return remoteAmount / (10 ** decimalsDiff);
}

// This is a safety check to prevent overflow in the next calculation.
// More than 77 would never fit in a uint256 and would cause an overflow. We also check if the resulting amount
// would overflow.
uint8 diffDecimals = i_tokenDecimals - remoteDecimals;
if (diffDecimals > 77 || remoteAmount > type(uint256).max / (10 ** diffDecimals)) {
revert OverflowDetected(remoteDecimals, i_tokenDecimals, remoteAmount);
}
return remoteAmount * (10 ** (i_tokenDecimals - remoteDecimals));

return remoteAmount * (10 ** diffDecimals);
}

// ================================================================
Expand Down
4 changes: 0 additions & 4 deletions contracts/src/v0.8/ccip/test/helpers/TokenPoolHelper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ contract TokenPoolHelper is TokenPool {
address router
) TokenPool(token, localTokenDecimals, allowlist, rmnProxy, router) {}

function getRemotePoolHashes() external view returns (bytes32[] memory) {
return new bytes32[](0); // s_remotePoolHashes.values();
}

function lockOrBurn(
Pool.LockOrBurnInV1 calldata lockOrBurnIn
) external override returns (Pool.LockOrBurnOutV1 memory) {
Expand Down
147 changes: 88 additions & 59 deletions contracts/src/v0.8/ccip/test/pools/TokenPool/TokenPool.addRemotePool.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.24;

import {Router} from "../../../Router.sol";
import {Pool} from "../../../libraries/Pool.sol";
import {TokenPool} from "../../../pools/TokenPool.sol";
import {TokenPoolSetup} from "./TokenPoolSetup.t.sol";
Expand All @@ -24,65 +25,93 @@ contract TokenPool_addRemotePool is TokenPoolSetup {
assertEq(remotePools[1], remotePool);
}

// function test_addRemotePool_MultipleActive() public {
// bytes[] memory remotePools = new bytes[](3);
// remotePools[0] = abi.encode(makeAddr("remotePool1"));
// remotePools[1] = abi.encode(makeAddr("remotePool2"));
// remotePools[2] = abi.encode(makeAddr("remotePool3"));
//
// address fakeOffRamp = makeAddr("fakeOffRamp");
//
// vm.mockCall(
// address(s_sourceRouter), abi.encodeCall(Router.isOffRamp, (DEST_CHAIN_SELECTOR, fakeOffRamp)), abi.encode(true)
// );
//
// vm.startPrank(fakeOffRamp);
//
// vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[0]));
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
//
// // There's already one pool setup through the test setup
// assertEq(s_tokenPool.getRemotePoolHashes().length, 1);
//
// vm.startPrank(OWNER);
// s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[0]);
//
// vm.startPrank(fakeOffRamp);
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
//
// // Adding an additional pool does not remove the previous one
// vm.startPrank(OWNER);
// s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[1]);
//
// // Both should now work
// assertEq(s_tokenPool.getRemotePoolHashes().length, 3);
// vm.startPrank(fakeOffRamp);
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));
//
// // Adding a third pool, and removing the first one
// vm.startPrank(OWNER);
// s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[2]);
// assertEq(s_tokenPool.getRemotePoolHashes().length, 4);
// s_tokenPool.removeRemotePool(DEST_CHAIN_SELECTOR, remotePools[0]);
// assertEq(s_tokenPool.getRemotePoolHashes().length, 3);
//
// // Only the last two should work
// vm.startPrank(fakeOffRamp);
// vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[0]));
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));
// s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[2]));
//
// // Removing the chain removes all associated pool hashes
// vm.startPrank(OWNER);
//
// uint64[] memory chainSelectorsToRemove = new uint64[](1);
// chainSelectorsToRemove[0] = DEST_CHAIN_SELECTOR;
// s_tokenPool.applyChainUpdates(chainSelectorsToRemove, new TokenPool.ChainUpdate[](0));
//
// assertEq(s_tokenPool.getRemotePoolHashes().length, 0);
// }
function test_addRemotePool_MultipleActive() public {
bytes[] memory remotePools = new bytes[](3);
remotePools[0] = abi.encode(makeAddr("remotePool1"));
remotePools[1] = abi.encode(makeAddr("remotePool2"));
remotePools[2] = abi.encode(makeAddr("remotePool3"));

address fakeOffRamp = makeAddr("fakeOffRamp");

vm.mockCall(
address(s_sourceRouter), abi.encodeCall(Router.isOffRamp, (DEST_CHAIN_SELECTOR, fakeOffRamp)), abi.encode(true)
);

vm.startPrank(fakeOffRamp);

vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[0]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));

// There's already one pool setup through the test setup
assertEq(s_tokenPool.getRemotePools(DEST_CHAIN_SELECTOR).length, 1);

vm.startPrank(OWNER);
s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[0]);

vm.startPrank(fakeOffRamp);
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));

// Adding an additional pool does not remove the previous one
vm.startPrank(OWNER);
s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[1]);

// Both should now work
assertEq(s_tokenPool.getRemotePools(DEST_CHAIN_SELECTOR).length, 3);
vm.startPrank(fakeOffRamp);
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));

// Adding a third pool, and removing the first one
vm.startPrank(OWNER);
s_tokenPool.addRemotePool(DEST_CHAIN_SELECTOR, remotePools[2]);
assertEq(s_tokenPool.getRemotePools(DEST_CHAIN_SELECTOR).length, 4);
s_tokenPool.removeRemotePool(DEST_CHAIN_SELECTOR, remotePools[0]);
assertEq(s_tokenPool.getRemotePools(DEST_CHAIN_SELECTOR).length, 3);

// Only the last two should work
vm.startPrank(fakeOffRamp);
vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[0]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[2]));

// Removing the chain removes all associated pool hashes
vm.startPrank(OWNER);

uint64[] memory chainSelectorsToRemove = new uint64[](1);
chainSelectorsToRemove[0] = DEST_CHAIN_SELECTOR;
s_tokenPool.applyChainUpdates(chainSelectorsToRemove, new TokenPool.ChainUpdate[](0));

assertEq(s_tokenPool.getRemotePools(DEST_CHAIN_SELECTOR).length, 0);

vm.expectRevert(abi.encodeWithSelector(TokenPool.ChainNotAllowed.selector, DEST_CHAIN_SELECTOR));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
vm.expectRevert(abi.encodeWithSelector(TokenPool.ChainNotAllowed.selector, DEST_CHAIN_SELECTOR));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));
vm.expectRevert(abi.encodeWithSelector(TokenPool.ChainNotAllowed.selector, DEST_CHAIN_SELECTOR));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[2]));

// Adding the chain back should NOT allow the previous pools to work again
TokenPool.ChainUpdate[] memory chainUpdate = new TokenPool.ChainUpdate[](1);
chainUpdate[0] = TokenPool.ChainUpdate({
remoteChainSelector: DEST_CHAIN_SELECTOR,
remotePoolAddresses: new bytes[](0),
remoteTokenAddress: abi.encode(s_initialRemoteToken),
outboundRateLimiterConfig: _getOutboundRateLimiterConfig(),
inboundRateLimiterConfig: _getInboundRateLimiterConfig()
});

vm.startPrank(OWNER);
s_tokenPool.applyChainUpdates(new uint64[](0), chainUpdate);

vm.startPrank(fakeOffRamp);
vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[0]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[0]));
vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[1]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[1]));
vm.expectRevert(abi.encodeWithSelector(TokenPool.InvalidSourcePoolAddress.selector, remotePools[2]));
s_tokenPool.releaseOrMint(_getReleaseOrMintInV1(remotePools[2]));
}

function _getReleaseOrMintInV1(
bytes memory sourcePoolAddress
Expand Down
Loading

0 comments on commit 22bcd4b

Please sign in to comment.