Add a bitmask of OutInstruction events to Executed

Allows explorers to provide clarity on what occurred.

processor/ethereum/router/contracts/IRouter.sol

@@ -22,7 +22,14 @@ interface IRouterWithoutCollisions {
   /// @notice Emitted when a batch of `OutInstruction`s occurs
   /// @param nonce The nonce consumed to execute this batch of transactions
   /// @param messageHash The hash of the message signed for the executed batch
-  event Executed(uint256 indexed nonce, bytes32 indexed messageHash);
+  /**
+   * @param results The result of each `OutInstruction` executed. This is a bitmask with true
+   *   representing success and false representing failure. The high bit (1 << 7) in the first byte
+   *   is used for the first `OutInstruction`, before the next bit, and so on, before the next byte.
+   *   An `OutInstruction` is considered as having succeeded if the call transferring ETH doesn't
+   *   fail, the ERC20 transfer doesn't fail, and any executed code doesn't revert.
+   */
+  event Executed(uint256 indexed nonce, bytes32 indexed messageHash, bytes results);
 
   /// @notice Emitted when `escapeHatch` is invoked
   /// @param escapeTo The address to escape to
@@ -36,11 +43,15 @@ interface IRouterWithoutCollisions {
   error EscapeHatchInvoked();
   /// @notice The signature was invalid
   error InvalidSignature();
+
   /// @notice The amount specified didn't match `msg.value`
   error AmountMismatchesMsgValue();
   /// @notice The call to an ERC20's `transferFrom` failed
   error TransferFromFailed();
 
+  /// @notice `execute` was re-entered
+  error ReenteredExecute();
+
   /// @notice An invalid address to escape to was specified.
   error InvalidEscapeAddress();
   /// @notice Escaping when escape hatch wasn't invoked.

processor/ethereum/router/contracts/Router.sol

@@ -25,6 +25,14 @@ import "IRouter.sol";
 /// @author Luke Parker <lukeparker@serai.exchange>
 /// @notice Intakes coins for the Serai network and handles relaying batches of transfers out
 contract Router is IRouterWithoutCollisions {
+  /// @dev The address in transient storage used for the reentrancy guard
+  bytes32 constant EXECUTE_REENTRANCY_GUARD_SLOT = bytes32(
+    /*
+      keccak256("ReentrancyGuard Router.execute") - 1
+    */
+    0xcf124a063de1614fedbd6b47187f98bf8873a1ae83da5c179a5881162f5b2401
+  );
+
   /**
    * @dev The next nonce used to determine the address of contracts deployed with CREATE. This is
    *   used to predict the addresses of deployed contracts ahead of time.
@@ -210,7 +218,8 @@ contract Router is IRouterWithoutCollisions {
         Require there was nothing returned, which is done by some non-standard tokens, or that the
         ERC20 contract did in fact return true
       */
-      bool nonStandardResOrTrue = (res.length == 0) || abi.decode(res, (bool));
+      bool nonStandardResOrTrue =
+        (res.length == 0) || ((res.length == 32) && abi.decode(res, (bool)));
       if (!(success && nonStandardResOrTrue)) revert TransferFromFailed();
     }
 
@@ -243,7 +252,16 @@ contract Router is IRouterWithoutCollisions {
   /// @param to The address to transfer the coins to
   /// @param coin The coin to transfer
   /// @param amount The amount of the coin to transfer
-  function erc20TransferOut(address to, address coin, uint256 amount) private {
+  /**
+   * @return success If the coins were successfully transferred out. This is defined as if the
+   *   call succeeded and returned true or nothing.
+   */
+  // execute has this annotation yet this still flags (even when it doesn't have its own loop)
+  // slither-disable-next-line calls-loop
+  function erc20TransferOut(address to, address coin, uint256 amount)
+    private
+    returns (bool success)
+  {
     /*
       The ERC20s integrated are presumed to have a constant gas cost, meaning this can only be
       insufficient:
@@ -262,48 +280,39 @@ contract Router is IRouterWithoutCollisions {
     */
     uint256 _gas = 100_000;
 
-    bytes memory _calldata = abi.encodeWithSelector(IERC20.transfer.selector, to, amount);
-    bool _success;
-    // slither-disable-next-line assembly
-    assembly {
     /*
-        `coin` is trusted so we can accept the risk of a return bomb here, yet we won't check the
-        return value anyways so there's no need to spend the gas decoding it. We assume failures
-        are the fault of the recipient, not us, the sender. We don't want to have such errors block
-        the queue of transfers to make.
-
-        If there ever was some invariant broken, off-chain actions is presumed to occur to move to a
-        new smart contract with whatever necessary changes made/response occurring.
+      `coin` is either signed (from `execute`) or called from `escape` (which can safely be
+      arbitrarily called). We accordingly don't need to be worried about return bombs here.
     */
-      _success :=
-        call(
-          _gas,
-          coin,
-          // Ether value
-          0,
-          // calldata
-          add(_calldata, 0x20),
-          mload(_calldata),
-          // return data
-          0,
-          0
-        )
-    }
+    // slither-disable-next-line return-bomb
+    (bool erc20Success, bytes memory res) =
+      address(coin).call{ gas: _gas }(abi.encodeWithSelector(IERC20.transfer.selector, to, amount));
+
+    /*
+      Require there was nothing returned, which is done by some non-standard tokens, or that the
+      ERC20 contract did in fact return true.
+    */
+    // slither-disable-next-line incorrect-equality
+    bool nonStandardResOrTrue = (res.length == 0) || ((res.length == 32) && abi.decode(res, (bool)));
+    success = erc20Success && nonStandardResOrTrue;
   }
 
   /// @dev Perform an ETH/ERC20 transfer out
   /// @param to The address to transfer the coins to
   /// @param coin The coin to transfer (address(0) if Ether)
   /// @param amount The amount of the coin to transfer
-  function transferOut(address to, address coin, uint256 amount) private {
+  /**
+   * @return success If the coins were successfully transferred out. For Ethereum, this is if the
+   *   call succeeded. For the ERC20, it's if the call succeeded and returned true or nothing.
+   */
+  function transferOut(address to, address coin, uint256 amount) private returns (bool success) {
     if (coin == address(0)) {
       // Enough gas to service the transfer and a minimal amount of logic
       uint256 _gas = 5_000;
       // This uses assembly to prevent return bombs
-      bool _success;
       // slither-disable-next-line assembly
       assembly {
-        _success :=
+        success :=
           call(
             _gas,
             to,
@@ -317,7 +326,7 @@ contract Router is IRouterWithoutCollisions {
           )
       }
     } else {
-      erc20TransferOut(to, coin, amount);
+      success = erc20TransferOut(to, coin, amount);
     }
   }
 
@@ -354,25 +363,38 @@ contract Router is IRouterWithoutCollisions {
   // @param fee The fee to pay (in coin) to the caller for their relaying of this batch
   // @param outs The `OutInstruction`s to act on
   // Each individual call is explicitly metered to ensure there isn't a DoS here
-  // slither-disable-next-line calls-loop
+  // slither-disable-next-line calls-loop,reentrancy-events
   function execute4DE42904() external {
+    /*
+      Prevent re-entrancy.
+
+      We emit a bitmask of which `OutInstruction`s succeeded. Doing that requires executing the
+      `OutInstruction`s, which may re-enter here. While our application of CEI with verifySignature
+      prevents replays, re-entrancy would allow out-of-order execution of batches (despite their
+      in-order start of execution) which isn't a headache worth dealing with.
+    */
+    bytes32 executeReentrancyGuardSlot = EXECUTE_REENTRANCY_GUARD_SLOT;
+    bytes32 priorEntered;
+    // slither-disable-next-line assembly
+    assembly {
+      priorEntered := tload(executeReentrancyGuardSlot)
+      tstore(executeReentrancyGuardSlot, 1)
+    }
+    if (priorEntered != bytes32(0)) {
+      revert ReenteredExecute();
+    }
+
     (uint256 nonceUsed, bytes memory args, bytes32 message) = verifySignature();
     (,, address coin, uint256 fee, IRouter.OutInstruction[] memory outs) =
       abi.decode(args, (bytes32, bytes32, address, uint256, IRouter.OutInstruction[]));
 
-    // TODO: Also include a bit mask here
-    emit Executed(nonceUsed, message);
-
-    /*
-      Since we don't have a re-entrancy guard, it is possible for instructions from later batches to
-      be executed before these instructions. This is deemed fine. We only require later batches be
-      relayed after earlier batches in order to form backpressure. This means if a batch has a fee
-      which isn't worth relaying the batch for, so long as later batches are sufficiently
-      worthwhile, every batch will be relayed.
-    */
+    // Define a bitmask to store the results of all following `OutInstruction`s
+    bytes memory results = new bytes((outs.length + 7) / 8);
 
     // slither-disable-next-line reentrancy-events
     for (uint256 i = 0; i < outs.length; i++) {
+      bool success = true;
+
       // If the destination is an address, we perform a direct transfer
       if (outs[i].destinationType == IRouter.DestinationType.Address) {
         /*
@@ -381,7 +403,7 @@ contract Router is IRouterWithoutCollisions {
           corrected batch using this nonce.
         */
         address destination = abi.decode(outs[i].destination, (address));
-        transferOut(destination, coin, outs[i].amount);
+        success = transferOut(destination, coin, outs[i].amount);
       } else {
         // Prepare the transfer
         uint256 ethValue = 0;
@@ -397,9 +419,23 @@ contract Router is IRouterWithoutCollisions {
           address nextAddress = address(
             uint160(uint256(keccak256(abi.encodePacked(address(this), _smartContractNonce))))
           );
-          erc20TransferOut(nextAddress, coin, outs[i].amount);
+
+          success = erc20TransferOut(nextAddress, coin, outs[i].amount);
         }
 
+        /*
+          If success is false, we presume it a fault with an ERC20, not with us, and move on. If we
+          reverted here, we'd halt the execution of every single batch (now and future). By simply
+          moving on, we may have reached some invariant with this specific ERC20, yet the project
+          entire isn't put into a halted state.
+
+          Since the recipient is a fresh account, this presumably isn't the recipient being
+          blacklisted (the most likely invariant upon the integration of a popular, standard ERC20).
+          That means there likely is some invariant with this integration to be resolved later.
+          Since reaching this invariant state requires an invariant, and for the reasons above, this
+          is accepted.
+        */
+        if (success) {
           (IRouter.CodeDestination memory destination) =
             abi.decode(outs[i].destination, (IRouter.CodeDestination));
 
@@ -410,15 +446,27 @@ contract Router is IRouterWithoutCollisions {
             Failures are considered the recipient's fault. We explicitly do not want the surface
             area/inefficiency of caching these for later attempted retires.
 
-          We don't have to worry about a return bomb here as this is our own function which doesn't
-          return any data.
+            We don't have to worry about a return bomb here as this is our own function which
+            doesn't return any data.
           */
-        address(this).call{ gas: destination.gasLimit, value: ethValue }(
+          (success,) = address(this).call{ gas: destination.gasLimit, value: ethValue }(
             abi.encodeWithSelector(Router.executeArbitraryCode.selector, destination.code)
           );
         }
       }
 
+      if (success) {
+        results[i / 8] |= bytes1(uint8(1 << (7 - (i % 8))));
+      }
+    }
+
+    /*
+      Emit execution with the status of all included events.
+
+      This is an effect after interactions yet we have a reentrancy guard making this safe.
+    */
+    emit Executed(nonceUsed, message, results);
+
     // Transfer the fee to the relayer
     transferOut(msg.sender, coin, fee);
   }

processor/ethereum/router/src/tests/mod.rs

@@ -22,6 +22,18 @@ use ethereum_deployer::Deployer;
 
 use crate::{Coin, OutInstructions, Router};
 
+#[test]
+fn execute_reentrancy_guard() {
+  let hash = alloy_core::primitives::keccak256(b"ReentrancyGuard Router.execute");
+  assert_eq!(
+    alloy_core::primitives::hex::encode(
+      (U256::from_be_slice(hash.as_ref()) - U256::from(1u8)).to_be_bytes::<32>()
+    ),
+    // Constant from the Router contract
+    "cf124a063de1614fedbd6b47187f98bf8873a1ae83da5c179a5881162f5b2401",
+  );
+}
+
 #[test]
 fn selector_collisions() {
   assert_eq!(
@@ -32,6 +44,10 @@ fn selector_collisions() {
     crate::_irouter_abi::IRouter::updateSeraiKeyCall::SELECTOR,
     crate::_router_abi::Router::updateSeraiKey5A8542A2Call::SELECTOR
   );
+  assert_eq!(
+    crate::_irouter_abi::IRouter::escapeHatchCall::SELECTOR,
+    crate::_router_abi::Router::escapeHatchDCDD91CCCall::SELECTOR
+  );
 }
 
 pub(crate) fn test_key() -> (Scalar, PublicKey) {