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

Author: mrice32

contracts/SpokePool.sol

@@ -127,7 +127,7 @@ abstract contract SpokePool is
 
     bytes32 public constant UPDATE_V3_DEPOSIT_DETAILS_HASH =
         keccak256(
-            "UpdateDepositDetails(uint256 depositId,uint256 originChainId,uint256 updatedOutputAmount,address updatedRecipient,bytes updatedMessage)"
+            "UpdateDepositDetails(uint32 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.
@@ -551,92 +551,59 @@ abstract contract SpokePool is
         uint32 exclusivityPeriod,
         bytes calldata message
     ) public payable override nonReentrant unpausedDeposits {
-        _depositV3(
-            depositor,
-            recipient,
+        // 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,
-            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(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(
+            uint32(currentTime) + exclusivityPeriod,
             depositor,
             recipient,
-            inputToken,
-            outputToken,
-            inputAmount,
-            outputAmount,
-            destinationChainId,
             exclusiveRelayer,
-            depositId,
-            quoteTimestamp,
-            fillDeadline,
-            uint32(getCurrentTime()) + exclusivityPeriod,
             message
         );
     }
@@ -790,7 +757,7 @@ abstract contract SpokePool is
      */
     function speedUpV3Deposit(
         address depositor,
-        uint256 depositId,
+        uint32 depositId,
         uint256 updatedOutputAmount,
         address updatedRecipient,
         bytes calldata updatedMessage,
@@ -1126,99 +1093,10 @@ 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,
@@ -1345,7 +1223,7 @@ abstract contract SpokePool is
 
     function _verifyUpdateV3DepositMessage(
         address depositor,
-        uint256 depositId,
+        uint32 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.
-        uint256 depositId;
+        uint32 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,
-        uint256 indexed depositId,
+        uint32 indexed depositId,
         uint32 quoteTimestamp,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
@@ -114,7 +114,7 @@ interface V3SpokePoolInterface {
 
     event RequestedSpeedUpV3Deposit(
         uint256 updatedOutputAmount,
-        uint256 indexed depositId,
+        uint32 indexed depositId,
         address indexed depositor,
         address updatedRecipient,
         bytes updatedMessage,
@@ -128,7 +128,7 @@ interface V3SpokePoolInterface {
         uint256 outputAmount,
         uint256 repaymentChainId,
         uint256 indexed originChainId,
-        uint256 indexed depositId,
+        uint32 indexed depositId,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
         address exclusiveRelayer,
@@ -145,7 +145,7 @@ interface V3SpokePoolInterface {
         uint256 inputAmount,
         uint256 outputAmount,
         uint256 indexed originChainId,
-        uint256 indexed depositId,
+        uint32 indexed depositId,
         uint32 fillDeadline,
         uint32 exclusivityDeadline,
         address exclusiveRelayer,
@@ -189,7 +189,7 @@ interface V3SpokePoolInterface {
 
     function speedUpV3Deposit(
         address depositor,
-        uint256 depositId,
+        uint32 depositId,
         uint256 updatedOutputAmount,
         address updatedRecipient,
         bytes calldata updatedMessage,

test/evm/hardhat/SpokePool.Deposit.ts

@@ -25,6 +25,7 @@ import {
   amountReceived,
   MAX_UINT32,
   originChainId,
+  zeroAddress,
 } from "./constants";
 
 const { AddressZero: ZERO_ADDRESS } = ethers.constants;
@@ -365,28 +366,6 @@ 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,
@@ -597,45 +576,6 @@ 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);