feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)

Author: nicholaspai

contracts/SpokePool.sol

@@ -125,7 +125,7 @@ abstract contract SpokePool is
 
     bytes32 public constant UPDATE_V3_DEPOSIT_DETAILS_HASH =
         keccak256(
-            "UpdateDepositDetails(uint32 depositId,uint256 originChainId,uint256 updatedOutputAmount,address updatedRecipient,bytes updatedMessage)"
+            "UpdateDepositDetails(uint256 depositId,uint256 originChainId,uint256 updatedOutputAmount,address updatedRecipient,bytes updatedMessage)"
         );
 
     // Default chain Id used to signify that no repayment is requested, for example when executing a slow fill.
@@ -548,59 +548,92 @@ abstract contract SpokePool is
         uint32 exclusivityPeriod,
         bytes calldata message
     ) public payable override nonReentrant unpausedDeposits {
-        // Check that deposit route is enabled for the input token. There are no checks required for the output token
-        // which is pulled from the relayer at fill time and passed through this contract atomically to the recipient.
-        if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute();
-
-        // Require that quoteTimestamp has a maximum age so that depositors pay an LP fee based on recent HubPool usage.
-        // It is assumed that cross-chain timestamps are normally loosely in-sync, but clock drift can occur. If the
-        // SpokePool time stalls or lags significantly, it is still possible to make deposits by setting quoteTimestamp
-        // within the configured buffer. The owner should pause deposits/fills if this is undesirable.
-        // This will underflow if quoteTimestamp is more than depositQuoteTimeBuffer;
-        // this is safe but will throw an unintuitive error.
-
-        // slither-disable-next-line timestamp
-        uint256 currentTime = getCurrentTime();
-        if (currentTime - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp();
-
-        // fillDeadline is relative to the destination chain.
-        // Don’t allow fillDeadline to be more than several bundles into the future.
-        // This limits the maximum required lookback for dataworker and relayer instances.
-        // Also, don't allow fillDeadline to be in the past. This poses a potential UX issue if the destination
-        // chain time keeping and this chain's time keeping are out of sync but is not really a practical hurdle
-        // unless they are significantly out of sync or the depositor is setting very short fill deadlines. This latter
-        // situation won't be a problem for honest users.
-        if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline();
-
-        // If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the
-        // transaction then the user is sending the native token. In this case, the native token should be
-        // wrapped.
-        if (inputToken == address(wrappedNativeToken) && msg.value > 0) {
-            if (msg.value != inputAmount) revert MsgValueDoesNotMatchInputAmount();
-            wrappedNativeToken.deposit{ value: msg.value }();
-            // Else, it is a normal ERC20. In this case pull the token from the caller as per normal.
-            // Note: this includes the case where the L2 caller has WETH (already wrapped ETH) and wants to bridge them.
-            // In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action.
-        } else {
-            // msg.value should be 0 if input token isn't the wrapped native token.
-            if (msg.value != 0) revert MsgValueDoesNotMatchInputAmount();
-            IERC20Upgradeable(inputToken).safeTransferFrom(msg.sender, address(this), inputAmount);
-        }
-
-        emit V3FundsDeposited(
+        _depositV3(
+            depositor,
+            recipient,
             inputToken,
             outputToken,
             inputAmount,
             outputAmount,
             destinationChainId,
+            exclusiveRelayer,
             // Increment count of deposits so that deposit ID for this spoke pool is unique.
+            // @dev Implicitly casts from uint32 to uint256 by padding the left-most bytes with zeros. Guarantees
+            // that the 24 most significant bytes are 0.
             numberOfDeposits++,
             quoteTimestamp,
             fillDeadline,
-            uint32(currentTime) + exclusivityPeriod,
+            uint32(getCurrentTime()) + exclusivityPeriod,
+            message
+        );
+    }
+
+    /**
+     * @notice See depositV3 for details. This function is identical to depositV3 except that it does not use the
+     * global deposit ID counter as a deposit nonce, instead allowing the caller to pass in a deposit nonce. This
+     * function is designed to be used by anyone who wants to pre-compute their resultant relay data hash, which
+     * could be useful for filling a deposit faster and avoiding any risk of a relay hash unexpectedly changing
+     * due to another deposit front-running this one and incrementing the global deposit ID counter.
+     * @dev This is labeled "unsafe" because there is no guarantee that the depositId emitted in the resultant
+     * V3FundsDeposited event is unique which means that the
+     * corresponding fill might collide with an existing relay hash on the destination chain SpokePool,
+     * which would make this deposit unfillable. In this case, the depositor would subsequently receive a refund
+     * of `inputAmount` of `inputToken` on the origin chain after the fill deadline.
+     * @dev On the destination chain, the hash of the deposit data will be used to uniquely identify this deposit, so
+     * modifying any params in it will result in a different hash and a different deposit. The hash will comprise
+     * all parameters to this function along with this chain's chainId(). Relayers are only refunded for filling
+     * deposits with deposit hashes that map exactly to the one emitted by this contract.
+     * @param depositNonce The nonce that uniquely identifies this deposit. This function will combine this parameter
+     * with the msg.sender address to create a unique uint256 depositNonce and ensure that the msg.sender cannot
+     * use this function to front-run another depositor's unsafe deposit. This function guarantees that the resultant
+     * deposit nonce will not collide with a safe uint256 deposit nonce whose 24 most significant bytes are always 0.
+     * @param depositor See identically named parameter in depositV3() comments.
+     * @param recipient See identically named parameter in depositV3() comments.
+     * @param inputToken See identically named parameter in depositV3() comments.
+     * @param outputToken See identically named parameter in depositV3() comments.
+     * @param inputAmount See identically named parameter in depositV3() comments.
+     * @param outputAmount See identically named parameter in depositV3() comments.
+     * @param destinationChainId See identically named parameter in depositV3() comments.
+     * @param exclusiveRelayer See identically named parameter in depositV3() comments.
+     * @param quoteTimestamp See identically named parameter in depositV3() comments.
+     * @param fillDeadline See identically named parameter in depositV3() comments.
+     * @param exclusivityPeriod See identically named parameter in depositV3() comments.
+     * @param message See identically named parameter in depositV3() comments.
+     */
+    function unsafeDepositV3(
+        address depositor,
+        address recipient,
+        address inputToken,
+        address outputToken,
+        uint256 inputAmount,
+        uint256 outputAmount,
+        uint256 destinationChainId,
+        address exclusiveRelayer,
+        uint256 depositNonce,
+        uint32 quoteTimestamp,
+        uint32 fillDeadline,
+        uint32 exclusivityPeriod,
+        bytes calldata message
+    ) public payable nonReentrant unpausedDeposits {
+        // @dev Create the uint256 deposit ID by concatenating the msg.sender and depositor address with the inputted
+        // depositNonce parameter. The resultant 32 byte string will be hashed and then casted to an "unsafe"
+        // uint256 deposit ID. The probability that the resultant ID collides with a "safe" deposit ID is
+        // equal to the chance that the first 28 bytes of the hash are 0, which is too small for us to consider.
+
+        uint256 depositId = getUnsafeDepositId(msg.sender, depositor, depositNonce);
+        _depositV3(
             depositor,
             recipient,
+            inputToken,
+            outputToken,
+            inputAmount,
+            outputAmount,
+            destinationChainId,
             exclusiveRelayer,
+            depositId,
+            quoteTimestamp,
+            fillDeadline,
+            uint32(getCurrentTime()) + exclusivityPeriod,
             message
         );
     }
@@ -754,7 +787,7 @@ abstract contract SpokePool is
      */
     function speedUpV3Deposit(
         address depositor,
-        uint32 depositId,
+        uint256 depositId,
         uint256 updatedOutputAmount,
         address updatedRecipient,
         bytes calldata updatedMessage,
@@ -1065,10 +1098,99 @@ abstract contract SpokePool is
         return block.timestamp; // solhint-disable-line not-rely-on-time
     }
 
+    /**
+     * @notice Returns the deposit ID for an unsafe deposit. This function is used to compute the deposit ID
+     * in unsafeDepositV3 and is provided as a convenience.
+     * @dev msgSenderand depositor are both used as inputs to allow passthrough depositors to create unique
+     * deposit hash spaces for unique depositors.
+     * @param msgSender The caller of the transaction used as input to produce the deposit ID.
+     * @param depositor The depositor address used as input to produce the deposit ID.
+     * @param depositNonce The nonce used as input to produce the deposit ID.
+     * @return The deposit ID for the unsafe deposit.
+     */
+    function getUnsafeDepositId(
+        address msgSender,
+        address depositor,
+        uint256 depositNonce
+    ) public pure returns (uint256) {
+        return uint256(keccak256(abi.encodePacked(msgSender, depositor, depositNonce)));
+    }
+
     /**************************************
      *         INTERNAL FUNCTIONS         *
      **************************************/
 
+    function _depositV3(
+        address depositor,
+        address recipient,
+        address inputToken,
+        address outputToken,
+        uint256 inputAmount,
+        uint256 outputAmount,
+        uint256 destinationChainId,
+        address exclusiveRelayer,
+        uint256 depositId,
+        uint32 quoteTimestamp,
+        uint32 fillDeadline,
+        uint32 exclusivityDeadline,
+        bytes calldata message
+    ) internal {
+        // Check that deposit route is enabled for the input token. There are no checks required for the output token
+        // which is pulled from the relayer at fill time and passed through this contract atomically to the recipient.
+        if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute();
+
+        // Require that quoteTimestamp has a maximum age so that depositors pay an LP fee based on recent HubPool usage.
+        // It is assumed that cross-chain timestamps are normally loosely in-sync, but clock drift can occur. If the
+        // SpokePool time stalls or lags significantly, it is still possible to make deposits by setting quoteTimestamp
+        // within the configured buffer. The owner should pause deposits/fills if this is undesirable.
+        // This will underflow if quoteTimestamp is more than depositQuoteTimeBuffer;
+        // this is safe but will throw an unintuitive error.
+
+        // slither-disable-next-line timestamp
+        uint256 currentTime = getCurrentTime();
+        if (currentTime - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp();
+
+        // fillDeadline is relative to the destination chain.
+        // Don’t allow fillDeadline to be more than several bundles into the future.
+        // This limits the maximum required lookback for dataworker and relayer instances.
+        // Also, don't allow fillDeadline to be in the past. This poses a potential UX issue if the destination
+        // chain time keeping and this chain's time keeping are out of sync but is not really a practical hurdle
+        // unless they are significantly out of sync or the depositor is setting very short fill deadlines. This latter
+        // situation won't be a problem for honest users.
+        if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline();
+
+        // If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the
+        // transaction then the user is sending the native token. In this case, the native token should be
+        // wrapped.
+        if (inputToken == address(wrappedNativeToken) && msg.value > 0) {
+            if (msg.value != inputAmount) revert MsgValueDoesNotMatchInputAmount();
+            wrappedNativeToken.deposit{ value: msg.value }();
+            // Else, it is a normal ERC20. In this case pull the token from the caller as per normal.
+            // Note: this includes the case where the L2 caller has WETH (already wrapped ETH) and wants to bridge them.
+            // In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action.
+        } else {
+            // msg.value should be 0 if input token isn't the wrapped native token.
+            if (msg.value != 0) revert MsgValueDoesNotMatchInputAmount();
+            IERC20Upgradeable(inputToken).safeTransferFrom(msg.sender, address(this), inputAmount);
+        }
+
+        emit V3FundsDeposited(
+            inputToken,
+            outputToken,
+            inputAmount,
+            outputAmount,
+            destinationChainId,
+            depositId,
+            quoteTimestamp,
+            fillDeadline,
+            exclusivityDeadline,
+            depositor,
+            recipient,
+            exclusiveRelayer,
+            message
+        );
+    }
+
     function _deposit(
         address depositor,
         address recipient,
@@ -1195,7 +1317,7 @@ abstract contract SpokePool is
 
     function _verifyUpdateV3DepositMessage(
         address depositor,
-        uint32 depositId,
+        uint256 depositId,
         uint256 originChainId,
         uint256 updatedOutputAmount,
         address updatedRecipient,

contracts/interfaces/V3SpokePoolInterface.sol

@@ -53,7 +53,7 @@ interface V3SpokePoolInterface {
         // Origin chain id.
         uint256 originChainId;
         // The id uniquely identifying this deposit on the origin chain.
-        uint32 depositId;
+        uint256 depositId;
         // The timestamp on the destination chain after which this deposit can no longer be filled.
         uint32 fillDeadline;
         // The timestamp on the destination chain after which any relayer can fill the deposit.
@@ -102,7 +102,7 @@ interface V3SpokePoolInterface {
         uint256 inputAmount,
         uint256 outputAmount,
         uint256 indexed destinationChainId,
-        uint32 indexed depositId,
+        uint256 indexed depositId,
         uint32 quoteTimestamp,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
@@ -114,7 +114,7 @@ interface V3SpokePoolInterface {
 
     event RequestedSpeedUpV3Deposit(
         uint256 updatedOutputAmount,
-        uint32 indexed depositId,
+        uint256 indexed depositId,
         address indexed depositor,
         address updatedRecipient,
         bytes updatedMessage,
@@ -128,7 +128,7 @@ interface V3SpokePoolInterface {
         uint256 outputAmount,
         uint256 repaymentChainId,
         uint256 indexed originChainId,
-        uint32 indexed depositId,
+        uint256 indexed depositId,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
         address exclusiveRelayer,
@@ -145,7 +145,7 @@ interface V3SpokePoolInterface {
         uint256 inputAmount,
         uint256 outputAmount,
         uint256 indexed originChainId,
-        uint32 indexed depositId,
+        uint256 indexed depositId,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
         address exclusiveRelayer,
@@ -189,7 +189,7 @@ interface V3SpokePoolInterface {
 
     function speedUpV3Deposit(
         address depositor,
-        uint32 depositId,
+        uint256 depositId,
         uint256 updatedOutputAmount,
         address updatedRecipient,
         bytes calldata updatedMessage,

test/evm/hardhat/SpokePool.Deposit.ts

@@ -25,7 +25,6 @@ import {
   amountReceived,
   MAX_UINT32,
   originChainId,
-  zeroAddress,
 } from "./constants";
 
 const { AddressZero: ZERO_ADDRESS } = ethers.constants;
@@ -366,6 +365,28 @@ describe("SpokePool Depositor Logic", async function () {
         _relayData.message,
       ];
     }
+    function getUnsafeDepositArgsFromRelayData(
+      _relayData: V3RelayData,
+      _depositId: string,
+      _destinationChainId = destinationChainId,
+      _quoteTimestamp = quoteTimestamp
+    ) {
+      return [
+        _relayData.depositor,
+        _relayData.recipient,
+        _relayData.inputToken,
+        _relayData.outputToken,
+        _relayData.inputAmount,
+        _relayData.outputAmount,
+        _destinationChainId,
+        _relayData.exclusiveRelayer,
+        _depositId,
+        _quoteTimestamp,
+        _relayData.fillDeadline,
+        _relayData.exclusivityDeadline,
+        _relayData.message,
+      ];
+    }
     beforeEach(async function () {
       relayData = {
         depositor: depositor.address,
@@ -576,6 +597,45 @@ describe("SpokePool Depositor Logic", async function () {
         "ReentrancyGuard: reentrant call"
       );
     });
+    it("unsafe deposit ID", async function () {
+      const currentTime = (await spokePool.getCurrentTime()).toNumber();
+      // new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {msg.sender, depositor, forcedDepositId}.
+      const forcedDepositId = "99";
+      const expectedDepositId = BigNumber.from(
+        ethers.utils.solidityKeccak256(
+          ["address", "address", "uint256"],
+          [depositor.address, recipient.address, forcedDepositId]
+        )
+      );
+      expect(await spokePool.getUnsafeDepositId(depositor.address, recipient.address, forcedDepositId)).to.equal(
+        expectedDepositId
+      );
+      // Note: we deliberately set the depositor != msg.sender to test that the hashing algorithm correctly includes
+      // both addresses in the hash.
+      await expect(
+        spokePool
+          .connect(depositor)
+          .unsafeDepositV3(
+            ...getUnsafeDepositArgsFromRelayData({ ...relayData, depositor: recipient.address }, forcedDepositId)
+          )
+      )
+        .to.emit(spokePool, "V3FundsDeposited")
+        .withArgs(
+          relayData.inputToken,
+          relayData.outputToken,
+          relayData.inputAmount,
+          relayData.outputAmount,
+          destinationChainId,
+          expectedDepositId,
+          quoteTimestamp,
+          relayData.fillDeadline,
+          currentTime,
+          recipient.address,
+          relayData.recipient,
+          relayData.exclusiveRelayer,
+          relayData.message
+        );
+    });
   });
   describe("speed up V3 deposit", function () {
     const updatedOutputAmount = amountToDeposit.add(1);