Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra check for decimal math #15419

Merged
merged 9 commits into from
Nov 28, 2024
Merged

Add extra check for decimal math #15419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8d6b2c
improve pool tests
RensR Nov 25, 2024
4f0ed6b
add decimal & overflow checks
RensR Nov 26, 2024
7d5bbd3
add test cov
RensR Nov 26, 2024
5d23882
gen wrappers
RensR Nov 26, 2024
723cd98
add test
RensR Nov 26, 2024
3938263
liq man snap
RensR Nov 26, 2024
4fd2f17
fix usdc test
RensR Nov 27, 2024
ae89834
changeset
RensR Nov 27, 2024
b2ec9b8
gas golf decimal check
RensR Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions contracts/.changeset/three-dogs-return.md
View file
Open in desktop
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
Open in desktop

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions contracts/gas-snapshots/liquiditymanager.gas-snapshot
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading