Manager: end execution early on replay protection (#101)

Co-authored-by: Rahul Maganti <rahulmaganti@rahuls-mbp.mynetworksettings.com>

Author: RahulMaganti47

src/Manager.sol

@@ -365,15 +365,17 @@ abstract contract Manager is
     }
 
     // @dev Mark a message as executed.
-    // This function will revert if the message has already been executed.
-    function _replayProtect(bytes32 digest) internal {
+    // This function will retuns `true` if the message has already been executed.
+    function _replayProtect(bytes32 digest) internal returns (bool) {
         // check if this message has already been executed
         if (isMessageExecuted(digest)) {
-            revert MessageAlreadyExecuted(digest);
+            return true;
         }
 
         // mark this message as executed
         _getMessageAttestationsStorage()[digest].executed = true;
+
+        return false;
     }
 
     /// @dev Called after a message has been sufficiently verified to execute the command in the message.
@@ -394,7 +396,14 @@ abstract contract Manager is
             revert MessageNotApproved(digest);
         }
 
-        _replayProtect(digest);
+        bool msgAlreadyExecuted = _replayProtect(digest);
+        if (msgAlreadyExecuted) {
+            // end execution early to mitigate the possibility of race conditions from endpoints
+            // attempting to deliver the same message when (threshold < number of endpoint messages)
+            // notify client (off-chain process) so they don't attempt redundant msg delivery
+            emit MessageAlreadyExecuted(message.sourceManager, digest);
+            return;
+        }
 
         EndpointStructs.NativeTokenTransfer memory nativeTokenTransfer =
             EndpointStructs.parseNativeTokenTransfer(message.payload);

src/interfaces/IManager.sol

@@ -6,7 +6,6 @@ import "../libraries/NormalizedAmount.sol";
 interface IManager {
     error DeliveryPaymentTooLow(uint256 requiredPayment, uint256 providedPayment);
     error TransferAmountHasDust(uint256 amount, uint256 dust);
-    error MessageAlreadyExecuted(bytes32 msgHash);
     error MessageNotApproved(bytes32 msgHash);
     error InvalidTargetChain(uint16 targetChain, uint16 thisChain);
     error ZeroAmount();

src/interfaces/IManagerEvents.sol

@@ -14,4 +14,5 @@ interface IManagerEvents {
     event ThresholdChanged(uint8 oldThreshold, uint8 threshold);
     event EndpointAdded(address endpoint, uint8 threshold);
     event EndpointRemoved(address endpoint, uint8 threshold);
+    event MessageAlreadyExecuted(bytes32 indexed sourceManager, bytes32 indexed msgHash);
 }

test/Manager.t.sol

@@ -486,12 +486,15 @@ contract TestManager is Test, IManagerEvents, IRateLimiterEvents {
         assertEq(token.balanceOf(address(user_B)), 50 * 10 ** (decimals - 8));
 
         // replay protection
-        vm.expectRevert(
-            abi.encodeWithSignature(
-                "MessageAlreadyExecuted(bytes32)", EndpointStructs.managerMessageDigest(m)
-            )
-        );
+        vm.recordLogs();
         e2.receiveMessage(message);
+        Vm.Log[] memory entries = vm.getRecordedLogs();
+
+        assertEq(entries.length, 2);
+        assertEq(entries[1].topics.length, 3);
+        assertEq(entries[1].topics[0], keccak256("MessageAlreadyExecuted(bytes32,bytes32)"));
+        assertEq(entries[1].topics[1], toWormholeFormat(address(manager)));
+        assertEq(entries[1].topics[2], bytes32(keccak256(message)));
     }
 
     // TODO:
@@ -1004,9 +1007,15 @@ contract TestManager is Test, IManagerEvents, IRateLimiterEvents {
         assertEq(token.balanceOf(address(user_B)), 50 * 10 ** (decimals - 8));
 
         // replay protection
-        bytes4 selector = bytes4(keccak256("MessageAlreadyExecuted(bytes32)"));
-        vm.expectRevert(abi.encodeWithSelector(selector, EndpointStructs.managerMessageDigest(m)));
+        vm.recordLogs();
         e2.receiveMessage(message);
+        Vm.Log[] memory entries = vm.getRecordedLogs();
+
+        assertEq(entries.length, 2);
+        assertEq(entries[1].topics.length, 3);
+        assertEq(entries[1].topics[0], keccak256("MessageAlreadyExecuted(bytes32,bytes32)"));
+        assertEq(entries[1].topics[1], toWormholeFormat(address(manager)));
+        assertEq(entries[1].topics[2], bytes32(keccak256(message)));
     }
 
     // === upgradeability
@@ -1053,13 +1062,16 @@ contract TestManager is Test, IManagerEvents, IRateLimiterEvents {
         endpoints = new IEndpointReceiver[](1);
         endpoints[0] = IEndpointReceiver(address(manager));
 
-        // replay protection
-        vm.expectRevert(
-            abi.encodeWithSignature(
-                "MessageAlreadyExecuted(bytes32)", EndpointStructs.managerMessageDigest(m)
-            )
-        );
+        vm.recordLogs();
+
         IEndpointReceiver(address(manager)).receiveMessage(message);
+        Vm.Log[] memory entries = vm.getRecordedLogs();
+
+        assertEq(entries.length, 1);
+        assertEq(entries[0].topics.length, 3);
+        assertEq(entries[0].topics[0], keccak256("MessageAlreadyExecuted(bytes32,bytes32)"));
+        assertEq(entries[0].topics[1], toWormholeFormat(address(manager)));
+        assertEq(entries[0].topics[2], bytes32(keccak256(message)));
 
         _attestEndpointsHelper(
             user_A, user_B, 1, NormalizedAmount.wrap(type(uint64).max), endpoints