Skip to content

Commit

Permalink
Static analysis fixes (#1147)
Browse files Browse the repository at this point in the history
This PR addresses various static analysis findings, mostly to reduce gas
and contract bytecode size. Both before and after use 3600
optimizations. Due to the saves space in the onRamp we can bump the
number of optimizations.

Each commit addresses a different change, and each commit contains the
gas snapshot to illustrate the impact of that particular change.


Before 

```
| EVM2EVMMultiOffRamp             | 24.665    | -0.089      |
| EVM2EVMMultiOnRamp              | 20.885    | 3.691       |
| EVM2EVMOffRamp                  | 23.092    | 1.484       |
| EVM2EVMOnRamp                   | 24.432    | 0.144       |

2E:test_E2E_3MessagesSuccess_gas() (gas: 1.104.821)
MultiRampsE2E:test_E2E_3MessagesSuccess_gas() (gas: 1.424.394)
```


After 

```
| EVM2EVMMultiOffRamp             | 24.538    | 0.038       |
| EVM2EVMMultiOnRamp              | 20.841    | 3.735       |
| EVM2EVMOffRamp                  | 22.968    | 1.608       |
| EVM2EVMOnRamp                   | 24.351    | 0.225       |

E2E:test_E2E_3MessagesSuccess_gas() (gas: 1.102.679)
MultiRampsE2E:test_E2E_3MessagesSuccess_gas() (gas: 1.422.669)
```


note: comment slightly outdated since the last merge. Overall diff
should still be the same

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
  • Loading branch information
1 parent 7d2cae3 commit f77e6f0
Show file tree
Hide file tree
Showing 31 changed files with 483 additions and 450 deletions.
569 changes: 285 additions & 284 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: 8899695)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 8894901)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 8822699)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPoolsSuccess_AlreadyFinalized() (gas: 8898695)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_MultiStageFinalization() (gas: 8893901)
LiquidityManager_rebalanceLiquidity:test_rebalanceBetweenPools_NativeRewrap() (gas: 8821699)
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: 3437651)
LiquidityManager_setLocalLiquidityContainer:test_setLocalLiquidityContainerSuccess() (gas: 3436651)
LiquidityManager_setMinimumLiquidity:test_OnlyOwnerReverts() (gas: 10925)
LiquidityManager_setMinimumLiquidity:test_setMinimumLiquiditySuccess() (gas: 36389)
LiquidityManager_withdrawERC20:test_withdrawERC20Reverts() (gas: 180359)
Expand Down
4 changes: 2 additions & 2 deletions contracts/scripts/native_solc_compile_all_ccip
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ echo " └───────────────────────
SOLC_VERSION="0.8.24"
OPTIMIZE_RUNS=26000
OPTIMIZE_RUNS_OFFRAMP=18000
OPTIMIZE_RUNS_ONRAMP=3600
OPTIMIZE_RUNS_MULTI_OFFRAMP=2400
OPTIMIZE_RUNS_ONRAMP=4100
OPTIMIZE_RUNS_MULTI_OFFRAMP=2500


SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
Expand Down
3 changes: 1 addition & 2 deletions contracts/src/v0.8/ccip/AggregateRateLimiter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ contract AggregateRateLimiter is OwnerIsCreator {
// The token bucket object that contains the bucket state.
RateLimiter.TokenBucket private s_rateLimiter;

/// @param config The RateLimiter.Config containing the capacity and refill rate
/// of the bucket, plus the admin address.
/// @param config The RateLimiter.Config
constructor(RateLimiter.Config memory config) {
s_rateLimiter = RateLimiter.TokenBucket({
rate: config.rate,
Expand Down
35 changes: 8 additions & 27 deletions contracts/src/v0.8/ccip/libraries/Internal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -131,19 +131,6 @@ library Internal {
/// When abi encoded, each token transfer takes up 7 slots, excl bytes contents.
uint256 public constant ANY_2_EVM_MESSAGE_FIXED_BYTES_PER_TOKEN = 32 * 7;

function _toAny2EVMMessage(
EVM2EVMMessage memory original,
Client.EVMTokenAmount[] memory destTokenAmounts
) internal pure returns (Client.Any2EVMMessage memory message) {
return Client.Any2EVMMessage({
messageId: original.messageId,
sourceChainSelector: original.sourceChainSelector,
sender: abi.encode(original.sender),
data: original.data,
destTokenAmounts: destTokenAmounts
});
}

bytes32 internal constant EVM_2_EVM_MESSAGE_HASH = keccak256("EVM2EVMMessageHashV2");

/// @dev Used to hash messages for single-lane ramps.
Expand Down Expand Up @@ -239,27 +226,21 @@ library Internal {
);
}

/// @dev We disallow the first 1024 addresses to never allow calling precompiles. It is extremely unlikely that
/// anyone would ever be able to generate an address in this range.
uint256 public constant PRECOMPILE_SPACE = 1024;

/// @notice This methods provides validation for parsing abi encoded addresses by ensuring the
/// address is within the EVM address space. If it isn't it will revert with an InvalidEVMAddress error, which
/// we can catch and handle more gracefully than a revert from abi.decode.
/// @return The address if it is valid, the function will revert otherwise.
function _validateEVMAddress(bytes memory encodedAddress) internal pure returns (address) {
if (encodedAddress.length != 32) revert InvalidEVMAddress(encodedAddress);
return _validateEVMAddressFromUint256(abi.decode(encodedAddress, (uint256)));
}

/// @dev We disallow the first 1024 addresses to never allow calling precompiles. It is extremely unlikely that
/// anyone would ever be able to generate an address in this range.
uint256 public constant PRECOMPILE_SPACE = 1024;

/// @notice This method provides a safe way to convert a uint256 to an address.
/// It will revert if the uint256 is not a valid EVM address, or a precompile address.
/// @return The address if it is valid, the function will revert otherwise.
function _validateEVMAddressFromUint256(uint256 encodedAddress) internal pure returns (address) {
if (encodedAddress > type(uint160).max || encodedAddress < PRECOMPILE_SPACE) {
revert InvalidEVMAddress(abi.encode(encodedAddress));
uint256 encodedAddressUint = abi.decode(encodedAddress, (uint256));
if (encodedAddressUint > type(uint160).max || encodedAddressUint < PRECOMPILE_SPACE) {
revert InvalidEVMAddress(encodedAddress);
}
return address(uint160(encodedAddress));
return address(uint160(encodedAddressUint));
}

/// @notice Enum listing the possible message execution states within
Expand Down
53 changes: 31 additions & 22 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
error AlreadyAttempted(uint64 sourceChainSelector, uint64 sequenceNumber);
error AlreadyExecuted(uint64 sourceChainSelector, uint64 sequenceNumber);
error ZeroChainSelectorNotAllowed();
error ExecutionError(bytes32 messageId, bytes error);
error ExecutionError(bytes32 messageId, bytes err);
error SourceChainNotEnabled(uint64 sourceChainSelector);
error TokenDataMismatch(uint64 sourceChainSelector, uint64 sequenceNumber);
error UnexpectedTokenData();
Expand All @@ -47,8 +47,8 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
error RootAlreadyCommitted(uint64 sourceChainSelector, bytes32 merkleRoot);
error InvalidRoot();
error CanOnlySelfCall();
error ReceiverError(bytes error);
error TokenHandlingError(bytes error);
error ReceiverError(bytes err);
error TokenHandlingError(bytes err);
error EmptyReport();
error CursedByRMN(uint64 sourceChainSelector);
error NotACompatiblePool(address notPool);
Expand Down Expand Up @@ -284,8 +284,10 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
for (uint256 msgIndex = 0; msgIndex < numMsgs; ++msgIndex) {
uint256 newLimit = msgGasLimitOverrides[msgIndex];
// Checks to ensure message cannot be executed with less gas than specified.
if (newLimit != 0 && newLimit < report.messages[msgIndex].gasLimit) {
revert InvalidManualExecutionGasLimit(report.sourceChainSelector, msgIndex, newLimit);
if (newLimit != 0) {
if (newLimit < report.messages[msgIndex].gasLimit) {
revert InvalidManualExecutionGasLimit(report.sourceChainSelector, msgIndex, newLimit);
}
}
}
}
Expand Down Expand Up @@ -417,11 +419,15 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
// FAILURE -> FAILURE no nonce bump
// FAILURE -> SUCCESS no nonce bump
// UNTOUCHED messages MUST be executed in order always
if (message.header.nonce > 0 && originalState == Internal.MessageExecutionState.UNTOUCHED) {
// If a nonce is not incremented, that means it was skipped, and we can ignore the message
if (
!INonceManager(i_nonceManager).incrementInboundNonce(sourceChainSelector, message.header.nonce, message.sender)
) continue;
if (message.header.nonce != 0) {
if (originalState == Internal.MessageExecutionState.UNTOUCHED) {
// If a nonce is not incremented, that means it was skipped, and we can ignore the message
if (
!INonceManager(i_nonceManager).incrementInboundNonce(
sourceChainSelector, message.header.nonce, message.sender
)
) continue;
}
}

// Although we expect only valid messages will be committed, we check again
Expand All @@ -438,19 +444,22 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
// Since it's hard to estimate whether manual execution will succeed, we
// revert the entire transaction if it fails. This will show the user if
// their manual exec will fail before they submit it.
if (
manualExecution && newState == Internal.MessageExecutionState.FAILURE
&& originalState != Internal.MessageExecutionState.UNTOUCHED
) {
// If manual execution fails, we revert the entire transaction, unless the originalState is UNTOUCHED as we
// would still be making progress by changing the state from UNTOUCHED to FAILURE.
revert ExecutionError(message.header.messageId, returnData);
if (manualExecution) {
if (newState == Internal.MessageExecutionState.FAILURE) {
if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
// If manual execution fails, we revert the entire transaction, unless the originalState is UNTOUCHED as we
// would still be making progress by changing the state from UNTOUCHED to FAILURE.
revert ExecutionError(message.header.messageId, returnData);
}
}
}

// The only valid prior states are UNTOUCHED and FAILURE (checked above)
// The only valid post states are FAILURE and SUCCESS (checked below)
if (newState != Internal.MessageExecutionState.FAILURE && newState != Internal.MessageExecutionState.SUCCESS) {
revert InvalidNewState(sourceChainSelector, message.header.sequenceNumber, newState);
if (newState != Internal.MessageExecutionState.SUCCESS) {
if (newState != Internal.MessageExecutionState.FAILURE) {
revert InvalidNewState(sourceChainSelector, message.header.sequenceNumber, newState);
}
}

emit ExecutionStateChanged(
Expand Down Expand Up @@ -826,8 +835,8 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
// 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(
abi.encodeWithSelector(
IPoolV1.releaseOrMint.selector,
abi.encodeCall(
IPoolV1.releaseOrMint,
Pool.ReleaseOrMintInV1({
originalSender: originalSender,
receiver: receiver,
Expand Down Expand Up @@ -857,7 +866,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
// transfer them to the final receiver. We use the _callWithExactGasSafeReturnData function because
// the token contracts are not considered trusted.
(success, returnData,) = CallWithExactGas._callWithExactGasSafeReturnData(
abi.encodeWithSelector(IERC20.transfer.selector, receiver, localAmount),
abi.encodeCall(IERC20.transfer, (receiver, localAmount)),
localToken,
s_dynamicConfig.maxTokenTransferGas,
Internal.GAS_FOR_CALL_EXACT_CHECK,
Expand Down
Loading

0 comments on commit f77e6f0

Please sign in to comment.