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

Jump to bottom

Static analysis fixes #1147

Merged
merged 21 commits into from
Jul 16, 2024
Merged

Static analysis fixes #1147

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2a3faa0
encodeCall over encodeWithSelector
RensR Jul 3, 2024
5f40500
add zero check in RegistryModuleOwnerCustom
RensR Jul 3, 2024
720824c
rn error to err
RensR Jul 3, 2024
6c05b28
save minor gas & contract size
RensR Jul 3, 2024
df59755
fix stale comment
RensR Jul 5, 2024
3243255
unfold if statements using && into two ifs
RensR Jul 5, 2024
c886fbb
inline single use func
RensR Jul 5, 2024
ac21b6a
inline single line func
RensR Jul 5, 2024
6e2a846
Increase compiler optimizations
RensR Jul 5, 2024
e1e6ee5
Merge 6e2a846d93ab8c636ce4fa6545a8e9043a32d355 into 2713d42a779f635a4…
RensR Jul 5, 2024
4116e29
Update gethwrappers
app-token-issuer-infra-releng[bot] Jul 5, 2024
178e44a
comment fix
RensR Jul 15, 2024
d999821
undo some != 0 vs > 0 changes
RensR Jul 15, 2024
1a917d2
Merge branch 'ccip-develop' into static-analysis
RensR Jul 15, 2024
10204bb
Merge 1a917d296aca1c8ae64496c9b94b658ee4584347 into 1da6ea640d2c934b9…
RensR Jul 15, 2024
54e6271
Update gethwrappers
app-token-issuer-infra-releng[bot] Jul 15, 2024
cb8c322
Merge branch 'ccip-develop' into static-analysis
RensR Jul 15, 2024
331f3ff
Merge branch 'static-analysis' of https://github.com/smartcontractkit…
RensR Jul 15, 2024
e24dd83
lower opts
RensR Jul 15, 2024
205a074
rm offRamp &&
RensR Jul 15, 2024
17fdce9
Merge branch 'ccip-develop' into static-analysis
RensR Jul 16, 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
531 changes: 266 additions & 265 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/src/v0.8/ccip/CommitStore.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ contract CommitStore is ICommitStore, ITypeAndVersion, OCR2Base {
CommitReport memory report = abi.decode(encodedReport, (CommitReport));

// Check if the report contains price updates
if (report.priceUpdates.tokenPriceUpdates.length > 0 || report.priceUpdates.gasPriceUpdates.length > 0) {
if (report.priceUpdates.tokenPriceUpdates.length != 0 || report.priceUpdates.gasPriceUpdates.length != 0) {
// Check for price staleness based on the epoch and round
if (s_latestPriceEpochAndRound < epochAndRound) {
// If prices are not stale, update the latest epoch and round
Expand Down
58 changes: 33 additions & 25 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
View file
Open in desktop
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 InvalidMessageId(bytes32 messageId);
Expand Down Expand Up @@ -283,8 +283,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) {
elatoskinas marked this conversation as resolved.
Show resolved Hide resolved
revert InvalidManualExecutionGasLimit(report.sourceChainSelector, msgIndex, newLimit);
if (newLimit != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: are the gas / contract size changes significant for splitting if statements? I find a && b && c much more readable in most cases

if (newLimit < report.messages[msgIndex].gasLimit) {
revert InvalidManualExecutionGasLimit(report.sourceChainSelector, msgIndex, newLimit);
}
}
}
}
Expand Down Expand Up @@ -410,13 +412,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.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.nonce, abi.encode(message.sender)
)
) continue;
if (message.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.nonce, abi.encode(message.sender)
)
) continue;
}
}

// Although we expect only valid messages will be committed, we check again
Expand All @@ -433,19 +437,23 @@ 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.messageId, returnData);
if (manualExecution) {
if (
newState == Internal.MessageExecutionState.FAILURE
&& originalState != Internal.MessageExecutionState.UNTOUCHED
) {
RensR marked this conversation as resolved.
Show resolved Hide resolved
// 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.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.sequenceNumber, newState);
if (newState != Internal.MessageExecutionState.SUCCESS) {
if (newState != Internal.MessageExecutionState.FAILURE) {
revert InvalidNewState(sourceChainSelector, message.sequenceNumber, newState);
}
}

emit ExecutionStateChanged(sourceChainSelector, message.sequenceNumber, message.messageId, newState, returnData);
Expand Down Expand Up @@ -493,7 +501,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
function executeSingleMessage(Internal.EVM2EVMMessage memory message, bytes[] memory offchainTokenData) external {
if (msg.sender != address(this)) revert CanOnlySelfCall();
Client.EVMTokenAmount[] memory destTokenAmounts = new Client.EVMTokenAmount[](0);
if (message.tokenAmounts.length > 0) {
if (message.tokenAmounts.length != 0) {
destTokenAmounts = _releaseOrMintTokens(
message.tokenAmounts,
abi.encode(message.sender),
Expand Down Expand Up @@ -818,8 +826,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(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saves gas AND adds type safety, no brainer

IPoolV1.releaseOrMint,
Pool.ReleaseOrMintInV1({
originalSender: originalSender,
receiver: receiver,
Expand Down Expand Up @@ -849,7 +857,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
90 changes: 52 additions & 38 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
error AlreadyExecuted(uint64 sequenceNumber);
error ZeroAddressNotAllowed();
error CommitStoreAlreadyInUse();
error ExecutionError(bytes error);
error ExecutionError(bytes err);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove shadowing the error type

error InvalidSourceChain(uint64 sourceChainSelector);
error MessageTooLarge(uint256 maxSize, uint256 actualSize);
error TokenDataMismatch(uint64 sequenceNumber);
Expand All @@ -49,8 +49,8 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
error InvalidManualExecutionGasLimit(uint256 index, uint256 newLimit);
error RootNotCommitted();
error CanOnlySelfCall();
error ReceiverError(bytes error);
error TokenHandlingError(bytes error);
error ReceiverError(bytes err);
error TokenHandlingError(bytes err);
error EmptyReport();
error CursedByRMN();
error InvalidMessageId();
Expand Down Expand Up @@ -205,9 +205,11 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
function getSenderNonce(address sender) external view returns (uint64 nonce) {
uint256 senderNonce = s_senderNonce[sender];

if (senderNonce == 0 && i_prevOffRamp != address(0)) {
// If OffRamp was upgraded, check if sender has a nonce from the previous OffRamp.
return IAny2EVMOffRamp(i_prevOffRamp).getSenderNonce(sender);
if (senderNonce == 0) {
if (i_prevOffRamp != address(0)) {
// If OffRamp was upgraded, check if sender has a nonce from the previous OffRamp.
return IAny2EVMOffRamp(i_prevOffRamp).getSenderNonce(sender);
}
}
return uint64(senderNonce);
}
Expand All @@ -226,7 +228,11 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
for (uint256 i = 0; i < numMsgs; ++i) {
uint256 newLimit = gasLimitOverrides[i];
// Checks to ensure message cannot be executed with less gas than specified.
if (newLimit != 0 && newLimit < report.messages[i].gasLimit) revert InvalidManualExecutionGasLimit(i, newLimit);
if (newLimit != 0) {
if (newLimit < report.messages[i].gasLimit) {
revert InvalidManualExecutionGasLimit(i, newLimit);
}
}
}

_execute(report, gasLimitOverrides);
Expand Down Expand Up @@ -308,26 +314,28 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
if (originalState != Internal.MessageExecutionState.UNTOUCHED) revert AlreadyAttempted(message.sequenceNumber);
}

if (message.nonce > 0) {
if (message.nonce != 0) {
// In the scenario where we upgrade offRamps, we still want to have sequential nonces.
// Referencing the old offRamp to check the expected nonce if none is set for a
// given sender allows us to skip the current message if it would not be the next according
// to the old offRamp. This preserves sequencing between updates.
uint64 prevNonce = s_senderNonce[message.sender];
if (prevNonce == 0 && i_prevOffRamp != address(0)) {
prevNonce = IAny2EVMOffRamp(i_prevOffRamp).getSenderNonce(message.sender);
if (prevNonce + 1 != message.nonce) {
// the starting v2 onramp nonce, i.e. the 1st message nonce v2 offramp is expected to receive,
// is guaranteed to equal (largest v1 onramp nonce + 1).
// if this message's nonce isn't (v1 offramp nonce + 1), then v1 offramp nonce != largest v1 onramp nonce,
// it tells us there are still messages inflight for v1 offramp
emit SkippedSenderWithPreviousRampMessageInflight(message.nonce, message.sender);
continue;
if (prevNonce == 0) {
if (i_prevOffRamp != address(0)) {
prevNonce = IAny2EVMOffRamp(i_prevOffRamp).getSenderNonce(message.sender);
if (prevNonce + 1 != message.nonce) {
// the starting v2 onramp nonce, i.e. the 1st message nonce v2 offramp is expected to receive,
// is guaranteed to equal (largest v1 onramp nonce + 1).
// if this message's nonce isn't (v1 offramp nonce + 1), then v1 offramp nonce != largest v1 onramp nonce,
// it tells us there are still messages inflight for v1 offramp
emit SkippedSenderWithPreviousRampMessageInflight(message.nonce, message.sender);
continue;
}
// Otherwise this nonce is indeed the "transitional nonce", that is
// all messages sent to v1 ramp have been executed by the DON and the sequence can resume in V2.
// Note if first time user in V2, then prevNonce will be 0, and message.nonce = 1, so this will be a no-op.
s_senderNonce[message.sender] = prevNonce;
}
// Otherwise this nonce is indeed the "transitional nonce", that is
// all messages sent to v1 ramp have been executed by the DON and the sequence can resume in V2.
// Note if first time user in V2, then prevNonce will be 0, and message.nonce = 1, so this will be a no-op.
s_senderNonce[message.sender] = prevNonce;
}

// UNTOUCHED messages MUST be executed in order always IF message.nonce > 0.
Expand Down Expand Up @@ -358,19 +366,23 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
// 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(returnData);
if (manualExecution) {
if (
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(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(message.sequenceNumber, newState);
// The only valid post states are SUCCESS and FAILURE (checked below)
if (newState != Internal.MessageExecutionState.SUCCESS) {
if (newState != Internal.MessageExecutionState.FAILURE) {
revert InvalidNewState(message.sequenceNumber, newState);
}
}

// Nonce changes per state transition.
Expand All @@ -379,8 +391,10 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
// UNTOUCHED -> SUCCESS nonce bump
// FAILURE -> FAILURE no nonce bump
// FAILURE -> SUCCESS no nonce bump
if (message.nonce > 0 && originalState == Internal.MessageExecutionState.UNTOUCHED) {
s_senderNonce[message.sender]++;
if (message.nonce != 0) {
if (originalState == Internal.MessageExecutionState.UNTOUCHED) {
s_senderNonce[message.sender]++;
}
}

emit ExecutionStateChanged(message.sequenceNumber, message.messageId, newState, returnData);
Expand Down Expand Up @@ -449,7 +463,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
function executeSingleMessage(Internal.EVM2EVMMessage memory message, bytes[] memory offchainTokenData) external {
if (msg.sender != address(this)) revert CanOnlySelfCall();
Client.EVMTokenAmount[] memory destTokenAmounts = new Client.EVMTokenAmount[](0);
if (message.tokenAmounts.length > 0) {
if (message.tokenAmounts.length != 0) {
destTokenAmounts = _releaseOrMintTokens(
message.tokenAmounts, abi.encode(message.sender), message.receiver, message.sourceTokenData, offchainTokenData
);
Expand Down Expand Up @@ -607,8 +621,8 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
// 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 @@ -638,7 +652,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
// 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 Expand Up @@ -685,7 +699,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
}
}

if (value > 0) _rateLimitValue(value);
if (value != 0) _rateLimitValue(value);

return destTokenAmounts;
}
Expand Down
21 changes: 12 additions & 9 deletions contracts/src/v0.8/ccip/onRamp/EVM2EVMMultiOnRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,12 +280,13 @@ contract EVM2EVMMultiOnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCre
// Since the DON has to pay for the extraData to be included on the destination chain, we cap the length of the
// extraData. This prevents gas bomb attacks on the NOPs. As destBytesOverhead accounts for both
// extraData and offchainData, this caps the worst case abuse to the number of bytes reserved for offchainData.
if (
poolReturnData.destPoolData.length > Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES
&& poolReturnData.destPoolData.length
if (poolReturnData.destPoolData.length > Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES) {
if (
poolReturnData.destPoolData.length
> s_tokenTransferFeeConfig[destChainSelector][tokenAndAmount.token].destBytesOverhead
) {
revert SourceTokenDataTooLarge(tokenAndAmount.token);
) {
revert SourceTokenDataTooLarge(tokenAndAmount.token);
}
}
// We validate the token address to ensure it is a valid EVM address
Internal._validateEVMAddress(poolReturnData.destTokenAddress);
Expand Down Expand Up @@ -338,7 +339,7 @@ contract EVM2EVMMultiOnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCre
);

// Only check token value if there are tokens
if (numberOfTokens > 0) {
if (numberOfTokens != 0) {
address messageValidator = s_dynamicConfig.messageValidator;
if (messageValidator != address(0)) {
try IMessageInterceptor(messageValidator).onOutboundMessage(destChainSelector, message) {}
Expand Down Expand Up @@ -429,8 +430,10 @@ contract EVM2EVMMultiOnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCre
}
if (gasLimit > uint256(destChainDynamicConfig.maxPerMsgGasLimit)) revert MessageGasLimitTooHigh();
if (numberOfTokens > uint256(destChainDynamicConfig.maxNumberOfTokensPerMsg)) revert UnsupportedNumberOfTokens();
if (destChainDynamicConfig.enforceOutOfOrder && !allowOutOfOrderExecution) {
revert ExtraArgOutOfOrderExecutionMustBeTrue();
if (!allowOutOfOrderExecution) {
if (destChainDynamicConfig.enforceOutOfOrder) {
revert ExtraArgOutOfOrderExecutionMustBeTrue();
}
}
}

Expand Down Expand Up @@ -541,7 +544,7 @@ contract EVM2EVMMultiOnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCre
uint256 premiumFee = 0;
uint32 tokenTransferGas = 0;
uint32 tokenTransferBytesOverhead = 0;
if (message.tokenAmounts.length > 0) {
if (message.tokenAmounts.length != 0) {
(premiumFee, tokenTransferGas, tokenTransferBytesOverhead) =
_getTokenTransferCost(destChainSelector, message.feeToken, feeTokenPrice, message.tokenAmounts);
} else {
Expand Down
Loading