-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f8d6b2c
improve pool tests
RensR 4f0ed6b
add decimal & overflow checks
RensR 7d5bbd3
add test cov
RensR 5d23882
gen wrappers
RensR 723cd98
add test
RensR 3938263
liq man snap
RensR 4fd2f17
fix usdc test
RensR ae89834
changeset
RensR b2ec9b8
gas golf decimal check
RensR File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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,5 @@ | ||
--- | ||
'@chainlink/contracts': patch | ||
--- | ||
|
||
extra validation on decimal logic in token pools |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
||
|
@@ -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); | ||
|
@@ -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. | ||
|
@@ -256,17 +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) { | ||
if (remoteDecimals - i_tokenDecimals > 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)); | ||
} | ||
|
||
// 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. | ||
if ( | ||
i_tokenDecimals - remoteDecimals > 77 | ||
|| remoteAmount > type(uint256).max / (10 ** (i_tokenDecimals - remoteDecimals)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do these calculations multiple times Example
|
||
) { | ||
revert OverflowDetected(remoteDecimals, i_tokenDecimals, remoteAmount); | ||
} | ||
|
||
return remoteAmount * (10 ** (i_tokenDecimals - remoteDecimals)); | ||
} | ||
|
||
|
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.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.