From 6508957cbc152fe29d46b525ad1015c90b0737e1 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 23 Jan 2025 00:03:54 -0500 Subject: [PATCH] Make a proper nonReentrant modifier A transaction couldn't call execute twice within a single TX prior. Now, it can. Also adds a bit more context to the escape hatch events/errors. --- .../ethereum/router/contracts/IRouter.sol | 11 ++- .../ethereum/router/contracts/Router.sol | 78 ++++++++++++------- 2 files changed, 56 insertions(+), 33 deletions(-) diff --git a/processor/ethereum/router/contracts/IRouter.sol b/processor/ethereum/router/contracts/IRouter.sol index c5e9a2fa..57994f8d 100644 --- a/processor/ethereum/router/contracts/IRouter.sol +++ b/processor/ethereum/router/contracts/IRouter.sol @@ -42,7 +42,8 @@ interface IRouterWithoutCollisions { /// @notice Emitted when coins escape through the escape hatch /// @param coin The coin which escaped - event Escaped(address indexed coin); + /// @param amount The amount which escaped + event Escaped(address indexed coin, uint256 amount); /// @notice The key for Serai was invalid /// @dev This is incomplete and not always guaranteed to be thrown upon an invalid key @@ -57,13 +58,17 @@ interface IRouterWithoutCollisions { /// @notice The call to an ERC20's `transferFrom` failed error TransferFromFailed(); - /// @notice `execute` was re-entered - error ReenteredExecute(); + /// @notice A non-reentrant function was re-entered + error Reentered(); /// @notice An invalid address to escape to was specified. error InvalidEscapeAddress(); + /// @notice The escape address wasn't a contract. + error EscapeAddressWasNotAContract(); /// @notice Escaping when escape hatch wasn't invoked. error EscapeHatchNotInvoked(); + /// @notice Escaping failed to transfer out. + error EscapeFailed(); /// @notice Transfer coins into Serai with an instruction /// @param coin The coin to transfer in (address(0) if Ether) diff --git a/processor/ethereum/router/contracts/Router.sol b/processor/ethereum/router/contracts/Router.sol index 526e1b9c..ade72a8c 100644 --- a/processor/ethereum/router/contracts/Router.sol +++ b/processor/ethereum/router/contracts/Router.sol @@ -29,8 +29,8 @@ contract Router is IRouterWithoutCollisions { bytes32 constant ACCOUNT_WITHOUT_CODE_CODEHASH = keccak256(""); /// @dev The address in transient storage used for the reentrancy guard - bytes32 constant EXECUTE_REENTRANCY_GUARD_SLOT = - bytes32(uint256(keccak256("ReentrancyGuard Router.execute")) - 1); + bytes32 constant REENTRANCY_GUARD_SLOT = + bytes32(uint256(keccak256("ReentrancyGuard Router")) - 1); /** * @dev The next nonce used to determine the address of contracts deployed with CREATE. This is @@ -64,6 +64,28 @@ contract Router is IRouterWithoutCollisions { /// @dev The address escaped to address private _escapedTo; + /// @dev Acquire the re-entrancy lock for the lifetime of this transaction + modifier nonReentrant() { + bytes32 reentrancyGuardSlot = REENTRANCY_GUARD_SLOT; + bytes32 priorEntered; + // slither-disable-next-line assembly + assembly { + priorEntered := tload(reentrancyGuardSlot) + tstore(reentrancyGuardSlot, 1) + } + if (priorEntered != bytes32(0)) { + revert Reentered(); + } + + _; + + // Clear the re-entrancy guard to allow multiple transactions to non-re-entrant functions within + // a transaction + assembly { + tstore(reentrancyGuardSlot, 0) + } + } + /// @dev Set the next Serai key. This does not read from/write to `_nextNonce` /// @param nonceUpdatedWith The nonce used to set the next key /// @param nextSeraiKeyVar The key to set as next @@ -139,15 +161,16 @@ contract Router is IRouterWithoutCollisions { bytes32 signatureS; // slither-disable-next-line assembly + uint256 chainID = block.chainid; assembly { // Read the signature (placed after the function signature) signatureC := mload(add(message, 36)) signatureS := mload(add(message, 68)) - // Overwrite the signature challenge with the nonce - mstore(add(message, 36), nonceUsed) - // Overwrite the signature response with 0 - mstore(add(message, 68), 0) + // Overwrite the signature challenge with the chain ID + mstore(add(message, 36), chainID) + // Overwrite the signature response with the nonce + mstore(add(message, 68), nonceUsed) // Calculate the message hash messageHash := keccak256(add(message, 32), messageLen) @@ -404,6 +427,12 @@ contract Router is IRouterWithoutCollisions { * fee. * * The hex bytes are to cause a function selector collision with `IRouter.execute`. + * + * Re-entrancy is prevented because 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 + * completion for the execution of batches (despite their in-order start of execution) which + * isn't a headache worth dealing with. */ // @param signature The signature by the current key for Serai's Ethereum validators // @param coin The coin all of these `OutInstruction`s are for @@ -411,26 +440,7 @@ contract Router is IRouterWithoutCollisions { // @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,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(); - } - + function execute4DE42904() external nonReentrant { (uint256 nonceUsed, bytes memory args, bytes32 message) = verifySignature(_seraiKey); (,, address coin, uint256 fee, IRouter.OutInstruction[] memory outs) = abi.decode(args, (bytes32, bytes32, address, uint256, IRouter.OutInstruction[])); @@ -545,11 +555,15 @@ contract Router is IRouterWithoutCollisions { The only check performed accordingly (with no confirmation flow) is that the new contract is in fact a contract. This is done to confirm the contract was successfully deployed on this blockchain. + + This check is also comprehensive to the zero-address case, but this function doesn't have to + be perfectly optimized and it's better to explicitly handle that due to it being its own + invariant. */ { bytes32 codehash = escapeTo.codehash; if ((codehash == bytes32(0)) || (codehash == ACCOUNT_WITHOUT_CODE_CODEHASH)) { - revert InvalidEscapeAddress(); + revert EscapeAddressWasNotAContract(); } } @@ -573,8 +587,6 @@ contract Router is IRouterWithoutCollisions { revert EscapeHatchNotInvoked(); } - emit Escaped(coin); - // Fetch the amount to escape uint256 amount = address(this).balance; if (coin != address(0)) { @@ -582,7 +594,13 @@ contract Router is IRouterWithoutCollisions { } // Perform the transfer - transferOut(_escapedTo, coin, amount); + // While this can be re-entered to try escaping our balance twice, the outer call will fail + if (!transferOut(_escapedTo, coin, amount)) { + revert EscapeFailed(); + } + + // Since we successfully escaped this amount, emit the event for it + emit Escaped(coin, amount); } /// @notice Fetch the next nonce to use by an action published to this contract