From b26c912026d03e446438ea84f3cba9957dd133d4 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 9 Jul 2024 13:27:52 -0400 Subject: [PATCH] better comments and natspec --- contracts/gas-snapshots/ccip.gas-snapshot | 46 +++++++------- .../ccip/applications/external/CCIPBase.sol | 40 +++++++----- .../ccip/applications/external/CCIPClient.sol | 38 +++++++---- .../external/CCIPClientWithACK.sol | 62 ++++++++++++++++-- .../applications/external/CCIPReceiver.sol | 6 +- .../external/CCIPReceiverWithACK.sol | 63 +++++++------------ .../external/CCIPReceiverTest.t.sol | 12 +++- 7 files changed, 166 insertions(+), 101 deletions(-) diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot index d418eded46..8c7653acfb 100644 --- a/contracts/gas-snapshots/ccip.gas-snapshot +++ b/contracts/gas-snapshots/ccip.gas-snapshot @@ -34,8 +34,8 @@ BurnWithFromMintTokenPool_lockOrBurn:test_ChainNotAllowed_Revert() (gas: 28675) BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurnRevertNotHealthy_Revert() (gas: 55158) BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurn_Success() (gas: 243568) BurnWithFromMintTokenPool_lockOrBurn:test_Setup_Success() (gas: 24260) -CCIPClientTest:test_ccipReceiveAndSendAck_Success() (gas: 331825) -CCIPClientTest:test_ccipSendAndReceiveAck_in_return_Success() (gas: 348272) +CCIPClientTest:test_ccipReceiveAndSendAck_Success() (gas: 331839) +CCIPClientTest:test_ccipSendAndReceiveAck_in_return_Success() (gas: 348275) CCIPClientTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens_Success() (gas: 241532) CCIPClientTest:test_send_tokens_that_are_not_feeToken_Success() (gas: 552173) CCIPConfigSetup:test_getCapabilityConfiguration_Success() (gas: 9495) @@ -88,18 +88,18 @@ CCIPConfig_validateConfig:test__validateConfig_TooManyBootstrapP2PIds_Reverts() CCIPConfig_validateConfig:test__validateConfig_TooManySigners_Reverts() (gas: 1160583) CCIPConfig_validateConfig:test__validateConfig_TooManyTransmitters_Reverts() (gas: 1158919) CCIPConfig_validateConfig:test_getCapabilityConfiguration_Success() (gas: 9562) -CCIPReceiverTest:test_HappyPath_Success() (gas: 193812) -CCIPReceiverTest:test_Recovery_from_invalid_sender_Success() (gas: 430948) +CCIPReceiverTest:test_HappyPath_Success() (gas: 193794) +CCIPReceiverTest:test_Recovery_from_invalid_sender_Success() (gas: 430905) CCIPReceiverTest:test_Recovery_with_intentional_Revert() (gas: 445121) -CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_Revert() (gas: 199816) -CCIPReceiverTest:test_modifyRouter_Success() (gas: 26470) -CCIPReceiverTest:test_removeSender_from_approvedList_and_revert_Success() (gas: 427992) -CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 421204) -CCIPReceiverTest:test_withdraw_nativeToken_to_owner_Success() (gas: 20689) -CCIPReceiverWithAckTest:test_ccipReceive_ack_message_Success() (gas: 56254) -CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack_Success() (gas: 331794) -CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_Revert() (gas: 438731) -CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor_Success() (gas: 2948564) +CCIPReceiverTest:test_disableChain_andRevert_onccipReceive_Revert() (gas: 199985) +CCIPReceiverTest:test_modifyRouter_Success() (gas: 26446) +CCIPReceiverTest:test_removeSender_from_approvedList_and_revert_Success() (gas: 427653) +CCIPReceiverTest:test_retryFailedMessage_Success() (gas: 454419) +CCIPReceiverTest:test_withdraw_nativeToken_to_owner_Success() (gas: 20667) +CCIPReceiverWithAckTest:test_ccipReceive_ack_message_Success() (gas: 56277) +CCIPReceiverWithAckTest:test_ccipReceive_and_respond_with_ack_Success() (gas: 331828) +CCIPReceiverWithAckTest:test_ccipReceiver_ack_with_invalidAckMessageHeaderBytes_Revert() (gas: 438737) +CCIPReceiverWithAckTest:test_feeTokenApproval_in_constructor_Success() (gas: 2956788) CCIPReceiverWithAckTest:test_modifyFeeToken_Success() (gas: 74835) CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andDestTokens() (gas: 339572) CCIPSenderTest:test_ccipSend_withNonNativeFeetoken_andNoDestTokens() (gas: 224467) @@ -208,7 +208,7 @@ EVM2EVMMultiOffRamp_executeSingleReport:test_NonExistingSourceChain_Revert() (ga EVM2EVMMultiOffRamp_executeSingleReport:test_ReceiverError_Success() (gas: 181628) EVM2EVMMultiOffRamp_executeSingleReport:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 190908) EVM2EVMMultiOffRamp_executeSingleReport:test_RootNotCommitted_Revert() (gas: 48050) -EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 436547) +EVM2EVMMultiOffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 1697359) EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain_Success() (gas: 252012) EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered_Success() (gas: 174226) EVM2EVMMultiOffRamp_executeSingleReport:test_SingleMessageNoTokens_Success() (gas: 193899) @@ -234,8 +234,8 @@ EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouche EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_FailedTx_Revert() (gas: 207956) EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ForkedChain_Revert() (gas: 26001) EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_GasLimitMismatchMultipleReports_Revert() (gas: 152913) -EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 507883) -EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2308282) +EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 1768860) +EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 3413460) EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_Success() (gas: 210050) EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride_Success() (gas: 210613) EVM2EVMMultiOffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride_Success() (gas: 670625) @@ -387,7 +387,7 @@ EVM2EVMOffRamp_execute:test_Paused_Revert() (gas: 101458) EVM2EVMOffRamp_execute:test_ReceiverError_Success() (gas: 165192) EVM2EVMOffRamp_execute:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 177948) EVM2EVMOffRamp_execute:test_RootNotCommitted_Revert() (gas: 41317) -EVM2EVMOffRamp_execute:test_RouterYULCall_Revert() (gas: 1678062) +EVM2EVMOffRamp_execute:test_RouterYULCall_Revert() (gas: 1663435) EVM2EVMOffRamp_execute:test_SingleMessageNoTokensUnordered_Success() (gas: 159863) EVM2EVMOffRamp_execute:test_SingleMessageNoTokens_Success() (gas: 175094) EVM2EVMOffRamp_execute:test_SingleMessageToNonCCIPReceiver_Success() (gas: 248764) @@ -419,14 +419,14 @@ EVM2EVMOffRamp_execute_upgrade:test_V2_Success() (gas: 131906) EVM2EVMOffRamp_getAllRateLimitTokens:test_GetAllRateLimitTokens_Success() (gas: 38408) EVM2EVMOffRamp_getExecutionState:test_FillExecutionState_Success() (gas: 3213556) EVM2EVMOffRamp_getExecutionState:test_GetExecutionState_Success() (gas: 83091) -EVM2EVMOffRamp_manuallyExecute:test_LowGasLimitManualExec_Success() (gas: 1759547) +EVM2EVMOffRamp_manuallyExecute:test_LowGasLimitManualExec_Success() (gas: 1744898) EVM2EVMOffRamp_manuallyExecute:test_ManualExecFailedTx_Revert() (gas: 186809) EVM2EVMOffRamp_manuallyExecute:test_ManualExecForkedChain_Revert() (gas: 25894) EVM2EVMOffRamp_manuallyExecute:test_ManualExecGasLimitMismatch_Revert() (gas: 43519) EVM2EVMOffRamp_manuallyExecute:test_ManualExecInvalidGasLimit_Revert() (gas: 26009) EVM2EVMOffRamp_manuallyExecute:test_ManualExecWithGasOverride_Success() (gas: 189003) EVM2EVMOffRamp_manuallyExecute:test_ManualExec_Success() (gas: 188464) -EVM2EVMOffRamp_manuallyExecute:test_ReentrancyManualExecuteFails() (gas: 3171279) +EVM2EVMOffRamp_manuallyExecute:test_ReentrancyManualExecuteFails() (gas: 3156679) EVM2EVMOffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched_Success() (gas: 144106) EVM2EVMOffRamp_metadataHash:test_MetadataHash_Success() (gas: 8871) EVM2EVMOffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 40429) @@ -693,8 +693,8 @@ OCR2Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 23484) OCR2Base_transmit:test_UnauthorizedSigner_Revert() (gas: 39665) OCR2Base_transmit:test_WrongNumberOfSignatures_Revert() (gas: 20557) OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 380711) -PingPong_example_ccipReceive:test_CcipReceive_Success() (gas: 308708) -PingPong_example_plumbing:test_Pausing_Success() (gas: 17766) +PingPong_example_ccipReceive:test_CcipReceive_Success() (gas: 307350) +PingPong_example_plumbing:test_Pausing_Success() (gas: 17898) PingPong_example_startPingPong:test_StartPingPong_Success() (gas: 234292) PriceRegistry_applyFeeTokensUpdates:test_ApplyFeeTokensUpdates_Success() (gas: 79823) PriceRegistry_applyFeeTokensUpdates:test_OnlyCallableByOwner_Revert() (gas: 12580) @@ -842,8 +842,8 @@ Router_routeMessage:test_ManualExec_Success() (gas: 35381) Router_routeMessage:test_OnlyOffRamp_Revert() (gas: 25116) Router_routeMessage:test_WhenNotHealthy_Revert() (gas: 44724) Router_setWrappedNative:test_OnlyOwner_Revert() (gas: 10985) -SelfFundedPingPong_ccipReceive:test_FundingIfNotANop_Revert() (gas: 290920) -SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 456179) +SelfFundedPingPong_ccipReceive:test_FundingIfNotANop_Revert() (gas: 289562) +SelfFundedPingPong_ccipReceive:test_Funding_Success() (gas: 448109) SelfFundedPingPong_setCountIncrBeforeFunding:test_setCountIncrBeforeFunding() (gas: 20203) TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_OnlyPendingAdministrator_Revert() (gas: 51085) TokenAdminRegistry_acceptAdminRole:test_acceptAdminRole_Success() (gas: 43947) diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol b/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol index 86ece0a049..205db1cd14 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPBase.sol @@ -23,9 +23,11 @@ abstract contract CCIPBase is OwnerIsCreator { event CCIPRouterModified(address indexed oldRouter, address indexed newRouter); event TokensWithdrawnByOwner(address indexed token, address indexed to, uint256 amount); - // TODO 2 events, add/remove - // TODO comment on the tradeoffs, short comment on why prefer indexed - event ApprovedSenderModified(uint64 indexed destChainSelector, bytes indexed recipient, bool isBeingApproved); + // Parameters are indexed to simplyify indexing of cross-chain dapps where contracts may be deployed with the same address. + // Since the updateApprovedSenders() function should be used sparingly by the contract owner, the additional gas cost should be negligible. If this function is needed to be used constantly, or with a large number of + // contracts, then an alternative and more gas-efficient method should be implemented instead, e.g. with merkle trees or indexing the parameters can be removed. + event ApprovedSenderAdded(uint64 indexed destChainSelector, bytes indexed recipient); + event ApprovedSenderRemoved(uint64 indexed destChainSelector, bytes indexed recipient); event ChainAdded(uint64 indexed remoteChainSelector, bytes indexed recipient, bytes extraArgsBytes); event ChainRemoved(uint64 indexed removeChainSelector); @@ -35,16 +37,16 @@ abstract contract CCIPBase is OwnerIsCreator { bytes sender; // The sender address on source chain that is allowed to call, ABI encoded in the case of a remote EVM chain } - struct ChainUpdate { // TODO comments - uint64 chainSelector; - bool allowed; - bytes recipient; - bytes extraArgsBytes; + struct ChainUpdate { + uint64 chainSelector; // ──╮ The unique identifier for a chain to send/receive messages + bool allowed; // ──╯ Whether the chain should be enabled + bytes recipient; // Address on the remote chain which should receive incoming messages from this. The should only be one per-chain + bytes extraArgsBytes; // Additional arguments to pass with the message including manually specifying gas limit and and whether to allow out-of-order execution } struct ChainConfig { bytes recipient; // The address to send messages to on the destination chain, ABI encoded in the case of a remote EVM chain. - bytes extraArgsBytes; // Specifies extraArgs to pass into ccipSend, includes configs such as gas limit, and OOO execution. + bytes extraArgsBytes; // Specifies extraArgs to pass into ccipSend, includes configs such as gas limit, and out-of-order execution. mapping(bytes recipient => bool isApproved) approvedSender; // Mapping is nested to support work-flows where Dapps may need to receive messages from one-or-more contracts on a source chain, or to support one-sided dapp upgrades. } @@ -81,19 +83,19 @@ abstract contract CCIPBase is OwnerIsCreator { function updateApprovedSenders( ApprovedSenderUpdate[] calldata adds, ApprovedSenderUpdate[] calldata removes - ) external onlyOwner { + ) external virtual onlyOwner { for (uint256 i = 0; i < removes.length; ++i) { delete s_chainConfigs[removes[i].destChainSelector].approvedSender[removes[i].sender]; // Third parameter is false to indicate that the sender's previous approval is being revoked, to improve off-chain event indexing - emit ApprovedSenderModified(removes[i].destChainSelector, removes[i].sender, false); + emit ApprovedSenderRemoved(removes[i].destChainSelector, removes[i].sender); } for (uint256 i = 0; i < adds.length; ++i) { s_chainConfigs[adds[i].destChainSelector].approvedSender[adds[i].sender] = true; // Third parameter is true to indicate that the sender is being approved, to improve off-chain event indexing - emit ApprovedSenderModified(adds[i].destChainSelector, adds[i].sender, true); + emit ApprovedSenderAdded(adds[i].destChainSelector, adds[i].sender); } } @@ -141,9 +143,10 @@ abstract contract CCIPBase is OwnerIsCreator { // │ Chain Management │ // ================================================================ - // TODO comments - // TODO name as updateRouter - function modifyRouter(address newRouter) external onlyOwner { + /// @notice Updates the address of the CCIP router to send/receive messages. + /// @dev function will can only be called by the owner, and should only be used in emergencies if the current CCIP Router is deprecated. + /// @param newRouter the address of the new router, cannot be the zero address. + function updateRouter(address newRouter) external onlyOwner { if (newRouter == address(0)) revert ZeroAddressNotAllowed(); // Store the old router in memory to emit event @@ -177,7 +180,8 @@ abstract contract CCIPBase is OwnerIsCreator { } } - // TODO comments + /// @notice Reverts if the specified chainSelector is not approved to send/receive messages to/from this contract + /// @param chainSelector the CCIP specific chain selector for a given remote-chain. modifier isValidChain(uint64 chainSelector) { // Must be storage and not memory because the struct contains a nested mapping which is not capable of being copied to memory ChainConfig storage currentConfig = s_chainConfigs[chainSelector]; @@ -185,6 +189,10 @@ abstract contract CCIPBase is OwnerIsCreator { _; } + /// @notice Ensures if the specified chain is not enabled, or if the sender of an incoming message has not been approved by contract owner + /// @param chainSelector the CCIP specific chain selector for a given remote-chain. + /// @param sender the address of the sender of the message on the source-chain. + /// @dev The modifier will revert if either the sender is not approved OR if the relevant chain is currently disabled. modifier isValidSender(uint64 chainSelector, bytes memory sender) { // If the chain is disabled, then short-circuit trigger a revert because no sender should be valid if (s_chainConfigs[chainSelector].recipient.length == 0 || !s_chainConfigs[chainSelector].approvedSender[sender]) { diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol b/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol index 6da2db8bd9..c97b69302b 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPClient.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.0; import {IRouterClient} from "../../interfaces/IRouterClient.sol"; import {Client} from "../../libraries/Client.sol"; -import {CCIPReceiverWithACK} from "./CCIPReceiverWithACK.sol"; +import {CCIPReceiver} from "./CCIPReceiver.sol"; import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; @@ -12,14 +12,23 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ /// @title CCIPClient /// @notice This contract implements logic for sending and receiving CCIP Messages. It utilizes CCIPReceiver's defensive patterns by default. /// @dev CCIPReceiver and CCIPSender cannot be simultaneously imported due to similar parents so CCIPSender functionality has been duplicated -// TODO make CCIPClient inherit from CCIPReceiver -contract CCIPClient is CCIPReceiverWithACK { +contract CCIPClient is CCIPReceiver { using SafeERC20 for IERC20; - constructor(address router, IERC20 feeToken) CCIPReceiverWithACK(router, feeToken) {} + IERC20 public s_feeToken; + + event MessageSent(bytes32 messageId); + + constructor(address router, IERC20 feeToken) CCIPReceiver(router) { + s_feeToken = feeToken; + } /// @notice sends a message through CCIP to the router - // TODO really beef up the comments here + /// @param destChainSelector The unique CCIP identifier for a destination chain + /// @param tokenAmounts An array of CCIP compatible tokens and their amounts to send through the bridge + /// @param data Arbitrary bytes to be sent to the destination address on the destination chain + /// @dev The recipient of the message and any extraArgs are set manually ahead of time using the applyChainUpdates() function in CCIPBase + /// @return messageId the unique message ID generated by the CCIP router and used to track message status. function ccipSend( uint64 destChainSelector, Client.EVMTokenAmount[] memory tokenAmounts, @@ -50,17 +59,24 @@ contract CCIPClient is CCIPReceiverWithACK { IERC20(s_feeToken).safeTransferFrom(msg.sender, address(this), fee); } - // TODO comment we only have messageId after calling ccipSend, so brekaing CEI is necessary - // messageId clac lives in OnRamp, which can be upgradaed, this it should be abstracted away from client impl + // messageId is only generated in the on-ramp, and therefore cannot be calcualated head of time. + // This necessitates breaking CEI but since the router is a trusted contract, any risks are negligible. messageId = IRouterClient(s_ccipRouter).ccipSend{value: address(s_feeToken) == address(0) ? fee : 0}( destChainSelector, message ); - s_messageStatus[messageId] = CCIPReceiverWithACK.MessageStatus.SENT; - - // Since the message was outgoing, and not ACK, reflect this with bytes32(0) - emit MessageSent(messageId, bytes32(0)); + emit MessageSent(messageId); return messageId; } + + /// @notice Contains arbitrary application-logic for incoming CCIP messages. + /// @dev It has to be external because of the try/catch of ccipReceive() which invokes it + function processMessage(Client.Any2EVMMessage calldata message) + external + override + onlySelf + isValidSender(message.sourceChainSelector, message.sender) + isValidChain(message.sourceChainSelector) + {} } diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPClientWithACK.sol b/contracts/src/v0.8/ccip/applications/external/CCIPClientWithACK.sol index 0c62f4ced9..0568f2fab2 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPClientWithACK.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPClientWithACK.sol @@ -1,8 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; +import {IRouterClient} from "../../interfaces/IRouterClient.sol"; import {Client} from "../../libraries/Client.sol"; -import {CCIPClient} from "./CCIPClient.sol"; +import {CCIPReceiverWithACK} from "./CCIPReceiverWithACK.sol"; import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; @@ -10,16 +11,69 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ /// @title CCIPClientWithACK /// @notice This contract implements logic for sending and receiving CCIP Messages, as well as responding to incoming messages with an ACK-response pattern. It utilizes CCIPReceiver's defensive patterns by default. /// @dev ccipSend functionality has been inherited from CCIPClient.sol, and _sendACK() from CCIPReceiverWithACK, so only processMessage() must be overridden to enable full functionality for processing incoming messages for ACK's -contract CCIPClientWithACK is CCIPClient { +contract CCIPClientWithACK is CCIPReceiverWithACK { using SafeERC20 for IERC20; error CannotAcknowledgeUnsentMessage(bytes32); - constructor(address router, IERC20 feeToken) CCIPClient(router, feeToken) {} + constructor(address router, IERC20 feeToken) CCIPReceiverWithACK(router, feeToken) {} + + /// @notice sends a message through CCIP to the router + /// @param destChainSelector The unique CCIP identifier for a destination chain + /// @param tokenAmounts An array of CCIP compatible tokens and their amounts to send through the bridge + /// @param data Arbitrary bytes to be sent to the destination address on the destination chain + /// @dev The recipient of the message and any extraArgs are set manually ahead of time using the applyChainUpdates() function in CCIPBase + /// @return messageId the unique message ID generated by the CCIP router and used to track message status. + function ccipSend( + uint64 destChainSelector, + Client.EVMTokenAmount[] memory tokenAmounts, + bytes memory data + ) public payable virtual isValidChain(destChainSelector) returns (bytes32 messageId) { + Client.EVM2AnyMessage memory message = Client.EVM2AnyMessage({ + receiver: s_chainConfigs[destChainSelector].recipient, + data: data, + tokenAmounts: tokenAmounts, + extraArgs: s_chainConfigs[destChainSelector].extraArgsBytes, + feeToken: address(s_feeToken) + }); + + for (uint256 i = 0; i < tokenAmounts.length; ++i) { + // Transfer the tokens to pay for tokens in tokenAmounts + IERC20(tokenAmounts[i].token).safeTransferFrom(msg.sender, address(this), tokenAmounts[i].amount); + + // Do not approve the tokens if it is the feeToken, otherwise the approval amount may overflow + if (tokenAmounts[i].token != address(s_feeToken)) { + IERC20(tokenAmounts[i].token).safeApprove(s_ccipRouter, tokenAmounts[i].amount); + } + } + + uint256 fee = IRouterClient(s_ccipRouter).getFee(destChainSelector, message); + + // Additional tokens for fees do not need to be approved to the router since it is already handled by setting s_feeToken + if (address(s_feeToken) != address(0)) { + IERC20(s_feeToken).safeTransferFrom(msg.sender, address(this), fee); + } + + messageId = IRouterClient(s_ccipRouter).ccipSend{value: address(s_feeToken) == address(0) ? fee : 0}( + destChainSelector, message + ); + + s_messageStatus[messageId] = CCIPReceiverWithACK.MessageStatus.SENT; + + emit MessageSent(messageId, bytes32(0)); + + return messageId; + } /// @notice Implementation of arbitrary logic to be executed when a CCIP message is received /// @dev is only invoked by self on CCIPReceive, and should implement arbitrary dapp-specific logic - function processMessage(Client.Any2EVMMessage calldata message) external virtual override onlySelf { + function processMessage(Client.Any2EVMMessage calldata message) + external + virtual + override + onlySelf + isValidSender(message.sourceChainSelector, message.sender) + { (MessagePayload memory payload) = abi.decode(message.data, (MessagePayload)); if (payload.messageType == MessageType.OUTGOING) { diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol index 4b38c6a2d6..7b9b0b97cc 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPReceiver.sol @@ -28,6 +28,7 @@ contract CCIPReceiver is CCIPBase { FAILED, // FAILED messages are messages which reverted during execution of processMessage() as part of the ccipReceive() try catch loop. ABANDONED // ABANDONED messages are ones which cannot be properly processed, but any sent tokens are recoverable, and can only be triggered by the contract owner. // Only a message that was previously marked as FAILED can be abandoned. + } // The message contents of failed messages are stored here. @@ -50,7 +51,7 @@ contract CCIPReceiver is CCIPBase { external virtual onlyRouter - isValidChain(message.sourceChainSelector) // TODO should it validate sender as well? + isValidChain(message.sourceChainSelector) { try this.processMessage(message) {} catch (bytes memory err) { @@ -111,8 +112,7 @@ contract CCIPReceiver is CCIPBase { /// @dev If the owner wants to retrieve tokens without special logic, then abandonFailedMessage(), withdrawNativeTokens(), or withdrawTokens() should be used instead /// This function is marked onlyOwner, but is virtual. Allowing permissionless execution is not recommended but may be allowed if function is overridden function _retryFailedMessage(Client.Any2EVMMessage memory message) internal virtual onlyOwner { - // TODO how about we add a default implementation that calls `processMessage`, and comments for overrides - // The idea is to vend an example that works somewhat well out of the box + this.processMessage(message); } /// @notice Should be used to recover tokens from a failed message, while ensuring the message cannot be retried diff --git a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol b/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol index 5b598a6e14..31d164fe1c 100644 --- a/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol +++ b/contracts/src/v0.8/ccip/applications/external/CCIPReceiverWithACK.sol @@ -11,7 +11,8 @@ import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/ import {EnumerableMap} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableMap.sol"; /// @title CCIPReceiverWithACK -/// TODO Beef this up +/// @notice Acts as a CCIP receiver, but upon receiving an incoming message, attempts to send a response back to the sender with an ACK packet indicating they received and processed the initial correspondence correctly. +/// @dev Messages received by this contract must be of special formatting in which any arbitrary data is first wrapped inside a MessagePayload struct, and must be processed first to ensure conformity. contract CCIPReceiverWithACK is CCIPReceiver { using SafeERC20 for IERC20; using EnumerableMap for EnumerableMap.Bytes32ToUintMap; @@ -21,29 +22,27 @@ contract CCIPReceiverWithACK is CCIPReceiver { event MessageAckSent(bytes32 incomingMessageId); event MessageSent(bytes32 indexed incomingMessageId, bytes32 indexed ACKMessageId); - // TODO named var - event MessageAckReceived(bytes32); + + event MessageAckReceived(bytes32 messageId); event FeeTokenModified(address indexed oldToken, address indexed newToken); - // TODO comments on OUTGOING vs. ACK, and OUTGOING is set in a child - // TODO consider moving this to CCIPClienWithACK because CCIPReceiver does not need to be aware of send/receives - // it is only concerned about receiving enum MessageType { - OUTGOING, - ACK + OUTGOING, // Indicates that a message is being sent for the first time to its recipient. + ACK // Indicates that another message of type "OUTGOING" has already been received, and an acknowledgement is being returned to the original sender, by the original recipient. + } - // TODO comments enum MessageStatus { - QUIET, - SENT, - ACKNOWLEDGED + QUIET, // A message which has not been sent yet, the default status for any messageId + SENT, // Indicates a message has been sent through CCIP but not yet received an ACK response from the recipient + ACKNOWLEDGED // The original SENT message was received and processed by the recipient, and confirmation of reception was received by this via the returned ACK message sent in response. + } struct MessagePayload { - bytes version; - bytes data; - MessageType messageType; + bytes version; // An optional byte string which can be used to denote the ACK version formatting or how to decode the remaining arbitrary data. + bytes data; // The Arbitrary data initially meant to be received by this contract and sent from the source chain. + MessageType messageType; // Denotes whether the incoming message is being received for the first time, or is an acknowledgement that the initial outgoing correspondence was successfully received. } string public constant ACK_MESSAGE_HEADER = "MESSAGE_ACKNOWLEDGED_"; @@ -79,38 +78,18 @@ contract CCIPReceiverWithACK is CCIPReceiver { emit FeeTokenModified(oldFeeToken, token); } - // TODO review if this just be inherited - /// @notice The entrypoint for the CCIP router to call. This function should never revert, all errors should be handled internally in this contract. - /// @param message The message to process. - /// @dev Extremely important to ensure only router calls this. - function ccipReceive(Client.Any2EVMMessage calldata message) - public - override - onlyRouter - isValidSender(message.sourceChainSelector, message.sender) - isValidChain(message.sourceChainSelector) - { - try this.processMessage(message) {} - catch (bytes memory err) { - s_failedMessages.set(message.messageId, uint256(ErrorCode.FAILED)); - s_messageContents[message.messageId] = message; - - // Don't revert so CCIPRouter doesn't revert. Emit event instead. - // The message can be retried later without having to do manual execution of CCIP. - emit MessageFailed(message.messageId, err); - return; - } - - emit MessageSucceeded(message.messageId); - } - /// @notice Application-specific logic for incoming ccip messages. /// @dev Function does NOT require the status of an incoming ACK be "sent" because this implementation does not send, only receives /// Any MessageType encoding is implemented by the sender contract, and is not natively part of CCIP messages. - function processMessage(Client.Any2EVMMessage calldata message) external virtual override onlySelf { + function processMessage(Client.Any2EVMMessage calldata message) + external + virtual + override + onlySelf + isValidSender(message.sourceChainSelector, message.sender) + { (MessagePayload memory payload) = abi.decode(message.data, (MessagePayload)); - // TODO CCIReceiverWithACK can just ack without message type checks // message type is a concept with ClientWithACK if (payload.messageType == MessageType.OUTGOING) { // Insert processing workflow here. diff --git a/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol b/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol index bac560975d..ec99aed57c 100644 --- a/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol +++ b/contracts/src/v0.8/ccip/test/applications/external/CCIPReceiverTest.t.sol @@ -186,6 +186,14 @@ contract CCIPReceiverTest is EVM2EVMOnRampSetup { vm.startPrank(OWNER); + // The message failed initially because the sender was not approved. Now we approve it and retry processing. Because retryFailedMessage() calls processMessage normally, it should execute successfully now. + CCIPBase.ApprovedSenderUpdate[] memory senderUpdates = new CCIPBase.ApprovedSenderUpdate[](1); + + senderUpdates[0] = + CCIPBase.ApprovedSenderUpdate({destChainSelector: sourceChainSelector, sender: abi.encode(address(1))}); + + s_receiver.updateApprovedSenders(senderUpdates, new CCIPBase.ApprovedSenderUpdate[](0)); + vm.expectEmit(); emit CCIPReceiver.MessageRecovered(messageId); @@ -261,14 +269,14 @@ contract CCIPReceiverTest is EVM2EVMOnRampSetup { function test_modifyRouter_Success() public { vm.expectRevert(abi.encodeWithSelector(CCIPBase.ZeroAddressNotAllowed.selector)); - s_receiver.modifyRouter(address(0)); + s_receiver.updateRouter(address(0)); address newRouter = address(0x1234); vm.expectEmit(); emit CCIPBase.CCIPRouterModified(address(s_destRouter), newRouter); - s_receiver.modifyRouter(newRouter); + s_receiver.updateRouter(newRouter); assertEq(s_receiver.getRouter(), newRouter, "Router Address not set correctly to the new router"); }