Remove revert cases in exec race conditions (#1246)

## Motivation Since the separation of manual exec window and DON exec window, it would theoretically be possible that two executions happen at the same time (although unlikely). ## Solution Remove reverts to ensure other msgs in the batches are unaffected --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
smartcontractkit · Aug 2, 2024 · eae928c · eae928c
1 parent de92ff0
commit eae928c
Show file tree

Hide file tree

Showing 9 changed files with 657 additions and 226 deletions.
diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot
diff --git a/contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol b/contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
@@ -33,7 +33,6 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
   using ERC165Checker for address;
   using EnumerableMapAddresses for EnumerableMapAddresses.AddressToAddressMap;
 
-  error AlreadyAttempted(uint64 sourceChainSelector, uint64 sequenceNumber);
   error AlreadyExecuted(uint64 sourceChainSelector, uint64 sequenceNumber);
   error ZeroChainSelectorNotAllowed();
   error ExecutionError(bytes32 messageId, bytes err);
@@ -74,6 +73,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
   event SourceChainSelectorAdded(uint64 sourceChainSelector);
   event SourceChainConfigSet(uint64 indexed sourceChainSelector, SourceChainConfig sourceConfig);
   event SkippedAlreadyExecutedMessage(uint64 sourceChainSelector, uint64 sequenceNumber);
+  event AlreadyAttempted(uint64 sourceChainSelector, uint64 sequenceNumber);
   /// @dev RMN depends on this event, if changing, please notify the RMN maintainers.
   event CommitReportAccepted(CommitReport report);
   event RootRemoved(bytes32 root);
@@ -375,13 +375,6 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
 
       Internal.MessageExecutionState originalState =
         getExecutionState(sourceChainSelector, message.header.sequenceNumber);
-      if (originalState == Internal.MessageExecutionState.SUCCESS) {
-        // If the message has already been executed, we skip it.  We want to not revert on race conditions between
-        // executing parties. This will allow us to open up manual exec while also attempting with the DON, without
-        // reverting an entire DON batch when a user manually executes while the tx is inflight.
-        emit SkippedAlreadyExecutedMessage(sourceChainSelector, message.header.sequenceNumber);
-        continue;
-      }
       // Two valid cases here, we either have never touched this message before, or we tried to execute
       // and failed. This check protects against reentry and re-execution because the other state is
       // IN_PROGRESS which should not be allowed to execute.
@@ -390,7 +383,13 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
           originalState == Internal.MessageExecutionState.UNTOUCHED
             || originalState == Internal.MessageExecutionState.FAILURE
         )
-      ) revert AlreadyExecuted(sourceChainSelector, message.header.sequenceNumber);
+      ) {
+        // If the message has already been executed, we skip it.  We want to not revert on race conditions between
+        // executing parties. This will allow us to open up manual exec while also attempting with the DON, without
+        // reverting an entire DON batch when a user manually executes while the tx is inflight.
+        emit SkippedAlreadyExecutedMessage(sourceChainSelector, message.header.sequenceNumber);
+        continue;
+      }
 
       if (manualExecution) {
         bool isOldCommitReport =
@@ -409,7 +408,8 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
         // DON can only execute a message once
         // Acceptable state transitions: UNTOUCHED->SUCCESS, UNTOUCHED->FAILURE
         if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
-          revert AlreadyAttempted(sourceChainSelector, message.header.sequenceNumber);
+          emit AlreadyAttempted(sourceChainSelector, message.header.sequenceNumber);
+          continue;
         }
       }
 

diff --git a/contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol b/contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol
@@ -34,7 +34,6 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
   using ERC165Checker for address;
   using EnumerableMapAddresses for EnumerableMapAddresses.AddressToAddressMap;
 
-  error AlreadyAttempted(uint64 sequenceNumber);
   error AlreadyExecuted(uint64 sequenceNumber);
   error ZeroAddressNotAllowed();
   error CommitStoreAlreadyInUse();
@@ -69,6 +68,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
   event TokenAggregateRateLimitAdded(address sourceToken, address destToken);
   event TokenAggregateRateLimitRemoved(address sourceToken, address destToken);
   event SkippedAlreadyExecutedMessage(uint64 indexed sequenceNumber);
+  event AlreadyAttempted(uint64 sequenceNumber);
 
   /// @notice Static offRamp config
   /// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
@@ -276,13 +276,6 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
     for (uint256 i = 0; i < numMsgs; ++i) {
       Internal.EVM2EVMMessage memory message = report.messages[i];
       Internal.MessageExecutionState originalState = getExecutionState(message.sequenceNumber);
-      if (originalState == Internal.MessageExecutionState.SUCCESS) {
-        // If the message has already been executed, we skip it.  We want to not revert on race conditions between
-        // executing parties. This will allow us to open up manual exec while also attempting with the DON, without
-        // reverting an entire DON batch when a user manually executes while the tx is inflight.
-        emit SkippedAlreadyExecutedMessage(message.sequenceNumber);
-        continue;
-      }
       // Two valid cases here, we either have never touched this message before, or we tried to execute
       // and failed. This check protects against reentry and re-execution because the other state is
       // IN_PROGRESS which should not be allowed to execute.
@@ -291,7 +284,13 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
           originalState == Internal.MessageExecutionState.UNTOUCHED
             || originalState == Internal.MessageExecutionState.FAILURE
         )
-      ) revert AlreadyExecuted(message.sequenceNumber);
+      ) {
+        // If the message has already been executed, we skip it.  We want to not revert on race conditions between
+        // executing parties. This will allow us to open up manual exec while also attempting with the DON, without
+        // reverting an entire DON batch when a user manually executes while the tx is inflight.
+        emit SkippedAlreadyExecutedMessage(message.sequenceNumber);
+        continue;
+      }
 
       if (manualExecution) {
         bool isOldCommitReport =
@@ -309,7 +308,10 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
       } else {
         // DON can only execute a message once
         // Acceptable state transitions: UNTOUCHED->SUCCESS, UNTOUCHED->FAILURE
-        if (originalState != Internal.MessageExecutionState.UNTOUCHED) revert AlreadyAttempted(message.sequenceNumber);
+        if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
+          emit AlreadyAttempted(message.sequenceNumber);
+          continue;
+        }
       }
 
       if (message.nonce != 0) {

diff --git a/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRamp.t.sol b/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRamp.t.sol
@@ -945,11 +945,10 @@ contract EVM2EVMMultiOffRamp_executeSingleReport is EVM2EVMMultiOffRampSetup {
     );
     s_offRamp.executeSingleReport(_generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), new uint256[](0));
 
-    vm.expectRevert(
-      abi.encodeWithSelector(
-        EVM2EVMMultiOffRamp.AlreadyAttempted.selector, SOURCE_CHAIN_SELECTOR_1, messages[0].header.sequenceNumber
-      )
-    );
+    // The second time should skip the msg
+    vm.expectEmit();
+    emit EVM2EVMMultiOffRamp.AlreadyAttempted(SOURCE_CHAIN_SELECTOR_1, messages[0].header.sequenceNumber);
+
     s_offRamp.executeSingleReport(_generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), new uint256[](0));
   }
 
@@ -1681,7 +1680,7 @@ contract EVM2EVMMultiOffRamp_manuallyExecute is EVM2EVMMultiOffRampSetup {
     s_offRamp.manuallyExecute(_generateBatchReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), gasLimitOverrides);
   }
 
-  function test_manuallyExecute_ReentrancyFails() public {
+  function test_manuallyExecute_ReentrancyFails_Success() public {
     uint256 tokenAmount = 1e9;
     IERC20 tokenToAbuse = IERC20(s_destFeeToken);
 
@@ -1715,29 +1714,26 @@ contract EVM2EVMMultiOffRamp_manuallyExecute is EVM2EVMMultiOffRampSetup {
     uint256[][] memory gasLimitOverrides = new uint256[][](1);
     gasLimitOverrides[0] = _getGasLimitsFromMessages(messages);
 
-    // The first entry should be fine and triggers the second entry. This one fails
-    // but since it's an inner tx of the first one it is caught in the try-catch.
-    // This means the first tx is marked `FAILURE` with the error message of the second tx.
+    // The first entry should be fine and triggers the second entry which is skipped. Due to the reentrancy
+    // the second completes first, so we expect the skip event before the success event.
+    vm.expectEmit();
+    emit EVM2EVMMultiOffRamp.SkippedAlreadyExecutedMessage(
+      messages[0].header.sourceChainSelector, messages[0].header.sequenceNumber
+    );
+
     vm.expectEmit();
     emit EVM2EVMMultiOffRamp.ExecutionStateChanged(
       messages[0].header.sourceChainSelector,
       messages[0].header.sequenceNumber,
       messages[0].header.messageId,
-      Internal.MessageExecutionState.FAILURE,
-      abi.encodeWithSelector(
-        EVM2EVMMultiOffRamp.ReceiverError.selector,
-        abi.encodeWithSelector(
-          EVM2EVMMultiOffRamp.AlreadyExecuted.selector,
-          messages[0].header.sourceChainSelector,
-          messages[0].header.sequenceNumber
-        )
-      )
+      Internal.MessageExecutionState.SUCCESS,
+      ""
     );
 
     s_offRamp.manuallyExecute(_generateBatchReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), gasLimitOverrides);
 
     // Since the tx failed we don't release the tokens
-    assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre);
+    assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre + tokenAmount);
   }
 }
 

diff --git a/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMOffRamp.t.sol b/contracts/src/v0.8/ccip/test/offRamp/EVM2EVMOffRamp.t.sol
@@ -633,6 +633,36 @@ contract EVM2EVMOffRamp_execute is EVM2EVMOffRampSetup {
     s_offRamp.execute(executionReport, new uint256[](0));
   }
 
+  function test_RetryFailedMessageWithoutManualExecution_Success() public {
+    Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
+
+    bytes memory realError1 = new bytes(2);
+    realError1[0] = 0xbe;
+    realError1[1] = 0xef;
+    s_reverting_receiver.setErr(realError1);
+
+    messages[0].receiver = address(s_reverting_receiver);
+    messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());
+
+    vm.expectEmit();
+    emit EVM2EVMOffRamp.ExecutionStateChanged(
+      messages[0].sequenceNumber,
+      messages[0].messageId,
+      Internal.MessageExecutionState.FAILURE,
+      abi.encodeWithSelector(
+        EVM2EVMOffRamp.ReceiverError.selector,
+        abi.encodeWithSelector(MaybeRevertMessageReceiver.CustomError.selector, realError1)
+      )
+    );
+    s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
+
+    // The second time should skip the msg
+    vm.expectEmit();
+    emit EVM2EVMOffRamp.AlreadyAttempted(messages[0].sequenceNumber);
+
+    s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
+  }
+
   // Reverts
 
   function test_InvalidMessageId_Revert() public {
@@ -744,33 +774,6 @@ contract EVM2EVMOffRamp_execute is EVM2EVMOffRampSetup {
     );
     s_offRamp.execute(executionReport, new uint256[](0));
   }
-
-  function test_RetryFailedMessageWithoutManualExecution_Revert() public {
-    Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
-
-    bytes memory realError1 = new bytes(2);
-    realError1[0] = 0xbe;
-    realError1[1] = 0xef;
-    s_reverting_receiver.setErr(realError1);
-
-    messages[0].receiver = address(s_reverting_receiver);
-    messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());
-
-    vm.expectEmit();
-    emit EVM2EVMOffRamp.ExecutionStateChanged(
-      messages[0].sequenceNumber,
-      messages[0].messageId,
-      Internal.MessageExecutionState.FAILURE,
-      abi.encodeWithSelector(
-        EVM2EVMOffRamp.ReceiverError.selector,
-        abi.encodeWithSelector(MaybeRevertMessageReceiver.CustomError.selector, realError1)
-      )
-    );
-    s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
-
-    vm.expectRevert(abi.encodeWithSelector(EVM2EVMOffRamp.AlreadyAttempted.selector, messages[0].sequenceNumber));
-    s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
-  }
 }
 
 contract EVM2EVMOffRamp_execute_upgrade is EVM2EVMOffRampSetup {
@@ -1144,6 +1147,55 @@ contract EVM2EVMOffRamp_manuallyExecute is EVM2EVMOffRampSetup {
     s_offRamp.manuallyExecute(_generateReportFromMessages(messages), gasLimitOverrides);
   }
 
+  function test_ReentrancyManualExecuteFails_Success() public {
+    uint256 tokenAmount = 1e9;
+    IERC20 tokenToAbuse = IERC20(s_destFeeToken);
+
+    // This needs to be deployed before the source chain message is sent
+    // because we need the address for the receiver.
+    ReentrancyAbuser receiver = new ReentrancyAbuser(address(s_destRouter), s_offRamp);
+    uint256 balancePre = tokenToAbuse.balanceOf(address(receiver));
+
+    // For this test any message will be flagged as correct by the
+    // commitStore. In a real scenario the abuser would have to actually
+    // send the message that they want to replay.
+    Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
+    messages[0].tokenAmounts = new Client.EVMTokenAmount[](1);
+    messages[0].tokenAmounts[0] = Client.EVMTokenAmount({token: s_sourceFeeToken, amount: tokenAmount});
+    messages[0].receiver = address(receiver);
+    messages[0].sourceTokenData = new bytes[](1);
+    messages[0].sourceTokenData[0] = abi.encode(
+      Internal.SourceTokenData({
+        sourcePoolAddress: abi.encode(s_sourcePoolByToken[s_sourceFeeToken]),
+        destTokenAddress: abi.encode(s_destTokenBySourceToken[s_sourceFeeToken]),
+        extraData: "",
+        destGasAmount: DEFAULT_TOKEN_DEST_GAS_OVERHEAD
+      })
+    );
+
+    messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());
+
+    Internal.ExecutionReport memory report = _generateReportFromMessages(messages);
+
+    // sets the report to be repeated on the ReentrancyAbuser to be able to replay
+    receiver.setPayload(report);
+
+    // The first entry should be fine and triggers the second entry which is skipped. Due to the reentrancy
+    // the second completes first, so we expect the skip event before the success event.
+    vm.expectEmit();
+    emit EVM2EVMOffRamp.SkippedAlreadyExecutedMessage(messages[0].sequenceNumber);
+
+    vm.expectEmit();
+    emit EVM2EVMOffRamp.ExecutionStateChanged(
+      messages[0].sequenceNumber, messages[0].messageId, Internal.MessageExecutionState.SUCCESS, ""
+    );
+
+    s_offRamp.manuallyExecute(report, _getGasLimitsFromMessages(messages));
+
+    // Assert that they only got the tokens once, not twice
+    assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre + tokenAmount);
+  }
+
   function test_ManualExecForkedChain_Revert() public {
     Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
 
@@ -1200,59 +1252,6 @@ contract EVM2EVMOffRamp_manuallyExecute is EVM2EVMOffRampSetup {
     );
     s_offRamp.manuallyExecute(_generateReportFromMessages(messages), _getGasLimitsFromMessages(messages));
   }
-
-  function test_ReentrancyManualExecuteFails() public {
-    uint256 tokenAmount = 1e9;
-    IERC20 tokenToAbuse = IERC20(s_destFeeToken);
-
-    // This needs to be deployed before the source chain message is sent
-    // because we need the address for the receiver.
-    ReentrancyAbuser receiver = new ReentrancyAbuser(address(s_destRouter), s_offRamp);
-    uint256 balancePre = tokenToAbuse.balanceOf(address(receiver));
-
-    // For this test any message will be flagged as correct by the
-    // commitStore. In a real scenario the abuser would have to actually
-    // send the message that they want to replay.
-    Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
-    messages[0].tokenAmounts = new Client.EVMTokenAmount[](1);
-    messages[0].tokenAmounts[0] = Client.EVMTokenAmount({token: s_sourceFeeToken, amount: tokenAmount});
-    messages[0].receiver = address(receiver);
-    messages[0].sourceTokenData = new bytes[](1);
-    messages[0].sourceTokenData[0] = abi.encode(
-      Internal.SourceTokenData({
-        sourcePoolAddress: abi.encode(s_sourcePoolByToken[s_sourceFeeToken]),
-        destTokenAddress: abi.encode(s_destTokenBySourceToken[s_sourceFeeToken]),
-        extraData: "",
-        destGasAmount: DEFAULT_TOKEN_DEST_GAS_OVERHEAD
-      })
-    );
-
-    messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());
-
-    Internal.ExecutionReport memory report = _generateReportFromMessages(messages);
-
-    // sets the report to be repeated on the ReentrancyAbuser to be able to replay
-    receiver.setPayload(report);
-
-    // The first entry should be fine and triggers the second entry. This one fails
-    // but since it's an inner tx of the first one it is caught in the try-catch.
-    // This means the first tx is marked `FAILURE` with the error message of the second tx.
-    vm.expectEmit();
-    emit EVM2EVMOffRamp.ExecutionStateChanged(
-      messages[0].sequenceNumber,
-      messages[0].messageId,
-      Internal.MessageExecutionState.FAILURE,
-      abi.encodeWithSelector(
-        EVM2EVMOffRamp.ReceiverError.selector,
-        abi.encodeWithSelector(EVM2EVMOffRamp.AlreadyExecuted.selector, messages[0].sequenceNumber)
-      )
-    );
-
-    s_offRamp.manuallyExecute(report, _getGasLimitsFromMessages(messages));
-
-    // Since the tx failed we don't release the tokens
-    assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre);
-  }
 }
 
 contract EVM2EVMOffRamp_getExecutionState is EVM2EVMOffRampSetup {

diff --git a/core/gethwrappers/ccip/generated/evm_2_evm_multi_offramp/evm_2_evm_multi_offramp.go b/core/gethwrappers/ccip/generated/evm_2_evm_multi_offramp/evm_2_evm_multi_offramp.go
diff --git a/core/gethwrappers/ccip/generated/evm_2_evm_offramp/evm_2_evm_offramp.go b/core/gethwrappers/ccip/generated/evm_2_evm_offramp/evm_2_evm_offramp.go