From 0688abd85f02f0e0eda5932d77d12bbaf40a4584 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 20 Aug 2024 11:08:27 -0400 Subject: [PATCH 01/10] feat(SpokePool): Introduce opt-in deterministic relay data hashes ## Motivation We should allow a depositor to pre-compute their relay data hash so that they can work with a filler to more quickly fill their deposit while avoiding re-org risk (which could change the ordering of V3FundsDeposited events and change a depositor's expected `depositId`) and race conditions with another innocent or malicious depositor who front-runs their depositId. The advantage of using the global `depositId` as a nonce today is gas cost savings since we don't need to store a new value in a new slot on each deposit. We should not add gas cost to deposits. Secondly, incrementing the global `depositId` after each deposit guarantees that each `depositV3()` produces a unique deposit. This means that a depositor does not have to worry about accidentally producing a relay hash collision that means their deposit would be unfillable. Fortunately, unfillable deposits are refunded on the origin chain to the depositor but this possibility is undesirable for many depositors. Therefore, any implementation of deterministic relay hashes should ideally be opt-in. ## Implementation Introduces a new function: `unsafeDepositV3()` which gives the msg.sender 12 bytes with which to set a deposit nonce. It is marked unsafe because there is no protection given by the contract that the resultant relay data hash produced will be unique. The 12 byte deposit nonce is combined with the msg.sender's address, allowing each msg.sender to only track their own deposit nonces to avoid collisions. This also protects depositors from being griefed by malicious actors who might try to front-run their unsafe relay data hashes. `unsafeDepositV3()` is no more expensive than `depositV3` from a gas-cost perspective. The existing `depositV3()` function's implementation is untouched. ## Changes to V3RelayData struct Changes the `depositId` parameter in the `V3RelayData` struct from uint32 to uint256. In storage, this parameter is stored as a uint256 so at the low-level, this changes nothing. Moreover, in practice until now, depositId's were always set as uint32 meaning that the first 24 bytes of the uint256 `depositId` slot are currently set to 0. This change is made because this PR introduces a new function: `unsafeDepositV3()` which allows someone to set a `depositId` with non-zero bytes in the first 24 bytes. --- contracts/SpokePool.sol | 231 +++++++++++++----- contracts/interfaces/V3SpokePoolInterface.sol | 12 +- 2 files changed, 181 insertions(+), 62 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index feb35a822..661ec7e6a 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -543,75 +543,107 @@ abstract contract SpokePool is uint32 exclusivityDeadline, 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(); - - // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period - // by forcing them to specify an exclusive relayer if the exclusivity period - // is in the future. If this deadline is 0, then the exclusive relayer must also be address(0). - // @dev Checks if either are > 0 by bitwise or-ing. - if (uint256(uint160(exclusiveRelayer)) | exclusivityDeadline != 0) { - // Now that we know one is nonzero, we need to perform checks on each. - // Check that exclusive relayer is nonzero. - if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); - - // Check that deadline is in the future. - if (exclusivityDeadline < currentTime) revert InvalidExclusivityDeadline(); - } - - // No need to sanity check exclusivityDeadline because if its bigger than fillDeadline, then - // there the full deadline is exclusive, and if its too small, then there is no exclusivity period. - - // 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, exclusivityDeadline, + message + ); + } + + // @dev Hashes the passed in depositId with the msg.sender address to ensure that msg.sender cannot use this + // function to front-run another depositor's unsafe deposit. Also sets the most significant byte to 1 + // to ensure that the resultant deposit nonce doesn't collide with a safe deposit nonce whose most + // significant byte is always 0. + + /** + * @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 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 This is slightly more gas efficient than the depositV3() function because it doesn't + * increment the global deposit count. + * @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 exclusivityDeadline 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, + uint96 depositNonce, + uint32 quoteTimestamp, + uint32 fillDeadline, + uint32 exclusivityDeadline, + bytes calldata message + ) public payable nonReentrant unpausedDeposits { + // @dev Create the uint256 deposit hash by first left shifting the address (which is 20 bytes) by 12 bytes. + // This creates room in the right-most 12 bytes for the passed in uint96 deposit nonce, which we set by + // bitwise OR-ing the left-shifted address with the nonce. We're then left with a 32 byte number. + // This guarantees the resultant hash won't collide with a safe deposit nonce which is set by + // implicitly casting a uint32 to a uint256, whose left-most 24 bytes are 0, while we're guaranteed that + // some of this deposit hash's first 24 bytes are not 0 due to the left shift of the msg.sender address, which + // won't be the zero address. + + // @dev For some L2's, it might actually be possible that the zero address can be the msg.sender which might + // make it possible for an unsafe deposit hash to collide with a safe deposit nonce. To prevent these cases, + // we might want to consider explicitly checking that the msg.sender is not the zero address. However, + // this is unlikely and we also should consider not checking it and incurring the extra gas cost: + // if (msg.sender == address()) revert InvalidUnsafeDepositor(); + + uint256 depositHash = uint256((uint160(msg.sender) << 12) | depositNonce); + _depositV3( depositor, recipient, + inputToken, + outputToken, + inputAmount, + outputAmount, + destinationChainId, exclusiveRelayer, + depositHash, + quoteTimestamp, + fillDeadline, + exclusivityDeadline, message ); } @@ -1077,6 +1109,93 @@ abstract contract SpokePool is * INTERNAL FUNCTIONS * **************************************/ + function _depositV3( + address depositor, + address recipient, + address inputToken, + address outputToken, + uint256 inputAmount, + uint256 outputAmount, + uint256 destinationChainId, + address exclusiveRelayer, + uint256 depositNonce, + 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(); + + // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period + // by forcing them to specify an exclusive relayer if the exclusivity period + // is in the future. If this deadline is 0, then the exclusive relayer must also be address(0). + // @dev Checks if either are > 0 by bitwise or-ing. + if (uint256(uint160(exclusiveRelayer)) | exclusivityDeadline != 0) { + // Now that we know one is nonzero, we need to perform checks on each. + // Check that exclusive relayer is nonzero. + if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); + + // Check that deadline is in the future. + if (exclusivityDeadline < currentTime) revert InvalidExclusivityDeadline(); + } + + // No need to sanity check exclusivityDeadline because if its bigger than fillDeadline, then + // there the full deadline is exclusive, and if its too small, then there is no exclusivity period. + + // 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, + depositNonce, + quoteTimestamp, + fillDeadline, + exclusivityDeadline, + depositor, + recipient, + exclusiveRelayer, + message + ); + } + function _deposit( address depositor, address recipient, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 4e4d4813e..821dec3b4 100644 --- a/contracts/interfaces/V3SpokePoolInterface.sol +++ b/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, From 28fcb30d338d11543f85f20df7c0596700baed0d Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 20 Aug 2024 11:37:06 -0400 Subject: [PATCH 02/10] change types --- contracts/SpokePool.sol | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 661ec7e6a..4e24ebb3c 100644 --- a/contracts/SpokePool.sol +++ b/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. @@ -563,11 +563,6 @@ abstract contract SpokePool is ); } - // @dev Hashes the passed in depositId with the msg.sender address to ensure that msg.sender cannot use this - // function to front-run another depositor's unsafe deposit. Also sets the most significant byte to 1 - // to ensure that the resultant deposit nonce doesn't collide with a safe deposit nonce whose most - // significant byte is always 0. - /** * @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 @@ -796,7 +791,7 @@ abstract contract SpokePool is */ function speedUpV3Deposit( address depositor, - uint32 depositId, + uint256 depositId, uint256 updatedOutputAmount, address updatedRecipient, bytes calldata updatedMessage, @@ -1322,7 +1317,7 @@ abstract contract SpokePool is function _verifyUpdateV3DepositMessage( address depositor, - uint32 depositId, + uint256 depositId, uint256 originChainId, uint256 updatedOutputAmount, address updatedRecipient, From 229f5eaa8ead1952003a6f92ac0e9b3077f48dfa Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 24 Sep 2024 10:24:34 -0400 Subject: [PATCH 03/10] Update SpokePool.sol --- contracts/SpokePool.sol | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 05a79792d..99845c581 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -563,7 +563,7 @@ abstract contract SpokePool is numberOfDeposits++, quoteTimestamp, fillDeadline, - exclusivityDeadline, + uint32(getCurrentTime()) + exclusivityPeriod, message ); } @@ -598,7 +598,7 @@ abstract contract SpokePool is * @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 exclusivityDeadline 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( @@ -613,24 +613,24 @@ abstract contract SpokePool is uint96 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, - uint32 exclusivityDeadline, + uint32 exclusivityPeriod, bytes calldata message ) public payable nonReentrant unpausedDeposits { - // @dev Create the uint256 deposit hash by first left shifting the address (which is 20 bytes) by 12 bytes. + // @dev Create the uint256 deposit ID by first left shifting the address (which is 20 bytes) by 12 bytes. // This creates room in the right-most 12 bytes for the passed in uint96 deposit nonce, which we set by // bitwise OR-ing the left-shifted address with the nonce. We're then left with a 32 byte number. - // This guarantees the resultant hash won't collide with a safe deposit nonce which is set by + // This guarantees the resultant ID won't collide with a safe deposit ID which is set by // implicitly casting a uint32 to a uint256, whose left-most 24 bytes are 0, while we're guaranteed that // some of this deposit hash's first 24 bytes are not 0 due to the left shift of the msg.sender address, which // won't be the zero address. - // @dev For some L2's, it might actually be possible that the zero address can be the msg.sender which might + // @dev For some L2's, it could be possible that the zero address can be the msg.sender which might // make it possible for an unsafe deposit hash to collide with a safe deposit nonce. To prevent these cases, // we might want to consider explicitly checking that the msg.sender is not the zero address. However, // this is unlikely and we also should consider not checking it and incurring the extra gas cost: - // if (msg.sender == address()) revert InvalidUnsafeDepositor(); + // if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); - uint256 depositHash = uint256((uint160(msg.sender) << 12) | depositNonce); + uint256 depositId = uint256((uint160(msg.sender) << 12) | depositNonce); _depositV3( depositor, recipient, @@ -640,10 +640,10 @@ abstract contract SpokePool is outputAmount, destinationChainId, exclusiveRelayer, - depositHash, + depositId, quoteTimestamp, fillDeadline, - exclusivityDeadline, + uint32(getCurrentTime()) + exclusivityPeriod, message ); } @@ -1121,7 +1121,7 @@ abstract contract SpokePool is uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, - uint256 depositNonce, + uint256 depositId, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityDeadline, @@ -1151,22 +1151,6 @@ abstract contract SpokePool is // situation won't be a problem for honest users. if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline(); - // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period - // by forcing them to specify an exclusive relayer if the exclusivity period - // is in the future. If this deadline is 0, then the exclusive relayer must also be address(0). - // @dev Checks if either are > 0 by bitwise or-ing. - if (uint256(uint160(exclusiveRelayer)) | exclusivityDeadline != 0) { - // Now that we know one is nonzero, we need to perform checks on each. - // Check that exclusive relayer is nonzero. - if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); - - // Check that deadline is in the future. - if (exclusivityDeadline < currentTime) revert InvalidExclusivityDeadline(); - } - - // No need to sanity check exclusivityDeadline because if its bigger than fillDeadline, then - // there the full deadline is exclusive, and if its too small, then there is no exclusivity period. - // 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. @@ -1188,7 +1172,7 @@ abstract contract SpokePool is inputAmount, outputAmount, destinationChainId, - depositNonce, + depositId, quoteTimestamp, fillDeadline, exclusivityDeadline, From 9e732fbd99122dcb5a059d2207f17b7c0d1711cd Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 24 Sep 2024 11:51:12 -0400 Subject: [PATCH 04/10] Use different hashing algorithm, add tests --- contracts/SpokePool.sol | 7 ++- test/evm/hardhat/SpokePool.Deposit.ts | 53 +++++++++++++++++++ .../evm/hardhat/fixtures/SpokePool.Fixture.ts | 2 +- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 99845c581..658c38c9a 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -616,9 +616,8 @@ abstract contract SpokePool is uint32 exclusivityPeriod, bytes calldata message ) public payable nonReentrant unpausedDeposits { - // @dev Create the uint256 deposit ID by first left shifting the address (which is 20 bytes) by 12 bytes. - // This creates room in the right-most 12 bytes for the passed in uint96 deposit nonce, which we set by - // bitwise OR-ing the left-shifted address with the nonce. We're then left with a 32 byte number. + // @dev Create the uint256 deposit ID by concatenating the address (which is 20 bytes) with the 12 byte + // depositNonce parameter. We're then left with a 32 byte number if we keccak256 the resultant packed bytes. // This guarantees the resultant ID won't collide with a safe deposit ID which is set by // implicitly casting a uint32 to a uint256, whose left-most 24 bytes are 0, while we're guaranteed that // some of this deposit hash's first 24 bytes are not 0 due to the left shift of the msg.sender address, which @@ -630,7 +629,7 @@ abstract contract SpokePool is // this is unlikely and we also should consider not checking it and incurring the extra gas cost: // if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); - uint256 depositId = uint256((uint160(msg.sender) << 12) | depositNonce); + uint256 depositId = uint256(keccak256(abi.encodePacked(msg.sender, depositNonce))); _depositV3( depositor, recipient, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 434917ecc..859733598 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -366,6 +366,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 +598,37 @@ 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 {address, forcedDepositId}. + const forcedDepositId = "99"; + const expectedDepositId = ethers.utils.solidityKeccak256( + ["address", "uint96"], + [depositor.address, forcedDepositId] + ); + // console.log(result.toString(), BigNumber.from(result)) + await expect( + spokePool + .connect(depositor) + .unsafeDepositV3(...getUnsafeDepositArgsFromRelayData({ ...relayData }, forcedDepositId)) + ) + .to.emit(spokePool, "V3FundsDeposited") + .withArgs( + relayData.inputToken, + relayData.outputToken, + relayData.inputAmount, + relayData.outputAmount, + destinationChainId, + BigNumber.from(expectedDepositId), + quoteTimestamp, + relayData.fillDeadline, + currentTime, + depositor.address, + relayData.recipient, + relayData.exclusiveRelayer, + relayData.message + ); + }); }); describe("speed up V3 deposit", function () { const updatedOutputAmount = amountToDeposit.add(1); diff --git a/test/evm/hardhat/fixtures/SpokePool.Fixture.ts b/test/evm/hardhat/fixtures/SpokePool.Fixture.ts index ac5d1d78d..393cba333 100644 --- a/test/evm/hardhat/fixtures/SpokePool.Fixture.ts +++ b/test/evm/hardhat/fixtures/SpokePool.Fixture.ts @@ -446,7 +446,7 @@ export async function getUpdatedV3DepositSignature( const typedData = { types: { UpdateDepositDetails: [ - { name: "depositId", type: "uint32" }, + { name: "depositId", type: "uint256" }, { name: "originChainId", type: "uint256" }, { name: "updatedOutputAmount", type: "uint256" }, { name: "updatedRecipient", type: "address" }, From 60589f1f42fc48167185f4fcdedff4f17cc7c355 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 25 Sep 2024 10:46:32 -0400 Subject: [PATCH 05/10] Remove the keccak256 The keccak256 hashing of the packed 32 bytes removes the guarantee that unsafe and safe deposits can't collide. This is because the resultanthash might produce a number less than uint32.max --- contracts/SpokePool.sol | 16 ++++++++-------- test/evm/hardhat/SpokePool.Deposit.ts | 6 +----- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 658c38c9a..b0fdd5506 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -617,19 +617,19 @@ abstract contract SpokePool is bytes calldata message ) public payable nonReentrant unpausedDeposits { // @dev Create the uint256 deposit ID by concatenating the address (which is 20 bytes) with the 12 byte - // depositNonce parameter. We're then left with a 32 byte number if we keccak256 the resultant packed bytes. - // This guarantees the resultant ID won't collide with a safe deposit ID which is set by - // implicitly casting a uint32 to a uint256, whose left-most 24 bytes are 0, while we're guaranteed that - // some of this deposit hash's first 24 bytes are not 0 due to the left shift of the msg.sender address, which - // won't be the zero address. + // depositNonce parameter. The resultant 32 byte string can be casted to an "unsafe" uint256 deposit ID. + // This guarantees the resultant ID won't collide with a "safe" deposit ID which is set by + // implicitly casting a uint32 to a uint256, where the left-most 24 bytes are set to 0. We guarantee + // that there are no collisions between "unsafe" and "safe" deposit ID's as long as the msg.sender's address + // is not the zero address. // @dev For some L2's, it could be possible that the zero address can be the msg.sender which might // make it possible for an unsafe deposit hash to collide with a safe deposit nonce. To prevent these cases, // we might want to consider explicitly checking that the msg.sender is not the zero address. However, - // this is unlikely and we also should consider not checking it and incurring the extra gas cost: - // if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); + // this is unlikely and we choose to not check it and incur the extra gas cost: + // e.g. if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); - uint256 depositId = uint256(keccak256(abi.encodePacked(msg.sender, depositNonce))); + uint256 depositId = uint256(bytes32(abi.encodePacked(msg.sender, depositNonce))); _depositV3( depositor, recipient, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 859733598..fde04c09c 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -602,11 +602,7 @@ describe("SpokePool Depositor Logic", async function () { const currentTime = (await spokePool.getCurrentTime()).toNumber(); // new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {address, forcedDepositId}. const forcedDepositId = "99"; - const expectedDepositId = ethers.utils.solidityKeccak256( - ["address", "uint96"], - [depositor.address, forcedDepositId] - ); - // console.log(result.toString(), BigNumber.from(result)) + const expectedDepositId = ethers.utils.solidityPack(["address", "uint96"], [depositor.address, forcedDepositId]); await expect( spokePool .connect(depositor) From 751ddf8e8ed1a6a87f07aa1f8fda69b23235e16c Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 25 Sep 2024 13:09:12 -0400 Subject: [PATCH 06/10] Add view function --- contracts/SpokePool.sol | 13 ++++++++++++- test/evm/hardhat/SpokePool.Deposit.ts | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index b0fdd5506..8fe40375a 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -629,7 +629,7 @@ abstract contract SpokePool is // this is unlikely and we choose to not check it and incur the extra gas cost: // e.g. if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); - uint256 depositId = uint256(bytes32(abi.encodePacked(msg.sender, depositNonce))); + uint256 depositId = getUnsafeDepositId(msg.sender, depositNonce); _depositV3( depositor, recipient, @@ -1107,6 +1107,17 @@ 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. + * @param depositor The 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 depositor, uint96 depositNonce) public pure returns (uint256) { + return uint256(bytes32(abi.encodePacked(depositor, depositNonce))); + } + /************************************** * INTERNAL FUNCTIONS * **************************************/ diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index fde04c09c..0347f232c 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -603,6 +603,7 @@ describe("SpokePool Depositor Logic", async function () { // new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {address, forcedDepositId}. const forcedDepositId = "99"; const expectedDepositId = ethers.utils.solidityPack(["address", "uint96"], [depositor.address, forcedDepositId]); + expect(await spokePool.getUnsafeDepositId(depositor.address, forcedDepositId)).to.equal(expectedDepositId); await expect( spokePool .connect(depositor) From 19a33801e31da303616ee12e192589941528eb1c Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 26 Sep 2024 10:49:52 -0400 Subject: [PATCH 07/10] Hash packed deterministic deposit id --- contracts/SpokePool.sol | 27 ++++++++++----------------- test/evm/hardhat/SpokePool.Deposit.ts | 4 ++-- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 8fe40375a..932580869 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -574,12 +574,11 @@ abstract contract SpokePool is * 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 is unique which means that the + * @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 This is slightly more gas efficient than the depositV3() function because it doesn't - * increment the global deposit count. * @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 @@ -610,24 +609,18 @@ abstract contract SpokePool is uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, - uint96 depositNonce, + uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityPeriod, bytes calldata message ) public payable nonReentrant unpausedDeposits { - // @dev Create the uint256 deposit ID by concatenating the address (which is 20 bytes) with the 12 byte + // @dev Create the uint256 deposit ID by concatenating the address with the inputted // depositNonce parameter. The resultant 32 byte string can be casted to an "unsafe" uint256 deposit ID. - // This guarantees the resultant ID won't collide with a "safe" deposit ID which is set by - // implicitly casting a uint32 to a uint256, where the left-most 24 bytes are set to 0. We guarantee - // that there are no collisions between "unsafe" and "safe" deposit ID's as long as the msg.sender's address - // is not the zero address. - - // @dev For some L2's, it could be possible that the zero address can be the msg.sender which might - // make it possible for an unsafe deposit hash to collide with a safe deposit nonce. To prevent these cases, - // we might want to consider explicitly checking that the msg.sender is not the zero address. However, - // this is unlikely and we choose to not check it and incur the extra gas cost: - // e.g. if (msg.sender == address(0)) revert InvalidUnsafeDepositor(); + // This 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. Moreover, if the depositId collided + // with an already emitted safe deposit ID, then the deposit would only be unfillable if the rest of the + // deposit data also matched, which would be very unlikely. uint256 depositId = getUnsafeDepositId(msg.sender, depositNonce); _depositV3( @@ -1114,8 +1107,8 @@ abstract contract SpokePool is * @param depositNonce The nonce used as input to produce the deposit ID. * @return The deposit ID for the unsafe deposit. */ - function getUnsafeDepositId(address depositor, uint96 depositNonce) public pure returns (uint256) { - return uint256(bytes32(abi.encodePacked(depositor, depositNonce))); + function getUnsafeDepositId(address depositor, uint256 depositNonce) public pure returns (uint256) { + return uint256(keccak256(abi.encodePacked(depositor, depositNonce))); } /************************************** diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 0347f232c..6a527853b 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -602,7 +602,7 @@ describe("SpokePool Depositor Logic", async function () { const currentTime = (await spokePool.getCurrentTime()).toNumber(); // new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {address, forcedDepositId}. const forcedDepositId = "99"; - const expectedDepositId = ethers.utils.solidityPack(["address", "uint96"], [depositor.address, forcedDepositId]); + const expectedDepositId = BigNumber.from(ethers.utils.solidityKeccak256(["address", "uint256"], [depositor.address, forcedDepositId])); expect(await spokePool.getUnsafeDepositId(depositor.address, forcedDepositId)).to.equal(expectedDepositId); await expect( spokePool @@ -616,7 +616,7 @@ describe("SpokePool Depositor Logic", async function () { relayData.inputAmount, relayData.outputAmount, destinationChainId, - BigNumber.from(expectedDepositId), + expectedDepositId, quoteTimestamp, relayData.fillDeadline, currentTime, From 635c5414524d3e23420b9c87220db8f309923341 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 26 Sep 2024 10:49:57 -0400 Subject: [PATCH 08/10] Update SpokePool.Deposit.ts --- test/evm/hardhat/SpokePool.Deposit.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 6a527853b..92cbe6845 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -602,7 +602,9 @@ describe("SpokePool Depositor Logic", async function () { const currentTime = (await spokePool.getCurrentTime()).toNumber(); // new deposit ID should be the uint256 equivalent of the keccak256 hash of packed {address, forcedDepositId}. const forcedDepositId = "99"; - const expectedDepositId = BigNumber.from(ethers.utils.solidityKeccak256(["address", "uint256"], [depositor.address, forcedDepositId])); + const expectedDepositId = BigNumber.from( + ethers.utils.solidityKeccak256(["address", "uint256"], [depositor.address, forcedDepositId]) + ); expect(await spokePool.getUnsafeDepositId(depositor.address, forcedDepositId)).to.equal(expectedDepositId); await expect( spokePool From 497a4932d37bfed96e0a66ed459d7e6eb6df2fc6 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 1 Oct 2024 11:02:32 -0400 Subject: [PATCH 09/10] Include both depositor and msg.sender in the hash --- contracts/SpokePool.sol | 27 ++++++++++++++++----------- test/evm/hardhat/SpokePool.Deposit.ts | 20 ++++++++++++++------ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 932580869..36c77f159 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -615,14 +615,12 @@ abstract contract SpokePool is uint32 exclusivityPeriod, bytes calldata message ) public payable nonReentrant unpausedDeposits { - // @dev Create the uint256 deposit ID by concatenating the address with the inputted - // depositNonce parameter. The resultant 32 byte string can be casted to an "unsafe" uint256 deposit ID. - // This 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. Moreover, if the depositId collided - // with an already emitted safe deposit ID, then the deposit would only be unfillable if the rest of the - // deposit data also matched, which would be very unlikely. - - uint256 depositId = getUnsafeDepositId(msg.sender, depositNonce); + // @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, @@ -1103,12 +1101,19 @@ abstract contract SpokePool is /** * @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. - * @param depositor The address used as input to produce the deposit ID. + * @dev msg.sender and depositor are both used as inputs to allow passthrough depositors to create unique + * deposit hash spaces for unique depositors. + * @param sender The msg.sender address 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 depositor, uint256 depositNonce) public pure returns (uint256) { - return uint256(keccak256(abi.encodePacked(depositor, depositNonce))); + function getUnsafeDepositId( + address sender, + address depositor, + uint256 depositNonce + ) public pure returns (uint256) { + return uint256(keccak256(abi.encodePacked(sender, depositor, depositNonce))); } /************************************** diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 92cbe6845..fd6b7a4e3 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -25,7 +25,6 @@ import { amountReceived, MAX_UINT32, originChainId, - zeroAddress, } from "./constants"; const { AddressZero: ZERO_ADDRESS } = ethers.constants; @@ -600,16 +599,25 @@ describe("SpokePool Depositor Logic", async function () { }); 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 {address, forcedDepositId}. + // 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", "uint256"], [depositor.address, forcedDepositId]) + ethers.utils.solidityKeccak256( + ["address", "address", "uint256"], + [depositor.address, recipient.address, forcedDepositId] + ) + ); + expect(await spokePool.getUnsafeDepositId(depositor.address, recipient.address, forcedDepositId)).to.equal( + expectedDepositId ); - expect(await spokePool.getUnsafeDepositId(depositor.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 }, forcedDepositId)) + .unsafeDepositV3( + ...getUnsafeDepositArgsFromRelayData({ ...relayData, depositor: recipient.address }, forcedDepositId) + ) ) .to.emit(spokePool, "V3FundsDeposited") .withArgs( @@ -622,7 +630,7 @@ describe("SpokePool Depositor Logic", async function () { quoteTimestamp, relayData.fillDeadline, currentTime, - depositor.address, + recipient.address, relayData.recipient, relayData.exclusiveRelayer, relayData.message From 78eb4c03d1954bc2515cbee25f07733d2c1aa1b3 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 2 Oct 2024 10:12:31 -0400 Subject: [PATCH 10/10] Update SpokePool.sol --- contracts/SpokePool.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 36c77f159..955812b82 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1101,19 +1101,19 @@ abstract contract SpokePool is /** * @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 msg.sender and depositor are both used as inputs to allow passthrough depositors to create unique + * @dev msgSenderand depositor are both used as inputs to allow passthrough depositors to create unique * deposit hash spaces for unique depositors. - * @param sender The msg.sender address used as input to produce the deposit ID. + * @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 sender, + address msgSender, address depositor, uint256 depositNonce ) public pure returns (uint256) { - return uint256(keccak256(abi.encodePacked(sender, depositor, depositNonce))); + return uint256(keccak256(abi.encodePacked(msgSender, depositor, depositNonce))); } /**************************************