From d363bf0e671082303f3a79cf8998848c766bd8dc Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 9 Oct 2024 01:57:29 -0400 Subject: [PATCH 01/23] Revert "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit 9d21d1ba12ef10751e1b33e09556b6c0b10883cc. --- contracts/SpokePool.sol | 210 ++++-------------- contracts/interfaces/V3SpokePoolInterface.sol | 12 +- test/evm/hardhat/SpokePool.Deposit.ts | 62 +----- .../evm/hardhat/fixtures/SpokePool.Fixture.ts | 2 +- 4 files changed, 52 insertions(+), 234 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 23f3a1a44..fe98700e7 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(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. @@ -549,92 +549,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 ); } @@ -788,7 +755,7 @@ abstract contract SpokePool is */ function speedUpV3Deposit( address depositor, - uint256 depositId, + uint32 depositId, uint256 updatedOutputAmount, address updatedRecipient, bytes calldata updatedMessage, @@ -1117,99 +1084,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, @@ -1336,7 +1214,7 @@ abstract contract SpokePool is function _verifyUpdateV3DepositMessage( address depositor, - uint256 depositId, + uint32 depositId, uint256 originChainId, uint256 updatedOutputAmount, address updatedRecipient, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index a5d09e0e3..0be81c630 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. - 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, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index fd6b7a4e3..434917ecc 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/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); diff --git a/test/evm/hardhat/fixtures/SpokePool.Fixture.ts b/test/evm/hardhat/fixtures/SpokePool.Fixture.ts index 393cba333..ac5d1d78d 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: "uint256" }, + { name: "depositId", type: "uint32" }, { name: "originChainId", type: "uint256" }, { name: "updatedOutputAmount", type: "uint256" }, { name: "updatedRecipient", type: "address" }, From c40c19771b87ce9394a06fd8b3c8f076fa02a153 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 9 Oct 2024 01:59:30 -0400 Subject: [PATCH 02/23] Reapply "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit d363bf0e671082303f3a79cf8998848c766bd8dc. --- contracts/SpokePool.sol | 210 ++++++++++++++---- contracts/interfaces/V3SpokePoolInterface.sol | 12 +- test/evm/hardhat/SpokePool.Deposit.ts | 62 +++++- .../evm/hardhat/fixtures/SpokePool.Fixture.ts | 2 +- 4 files changed, 234 insertions(+), 52 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index fe98700e7..23f3a1a44 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. @@ -549,59 +549,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 ); } @@ -755,7 +788,7 @@ abstract contract SpokePool is */ function speedUpV3Deposit( address depositor, - uint32 depositId, + uint256 depositId, uint256 updatedOutputAmount, address updatedRecipient, bytes calldata updatedMessage, @@ -1084,10 +1117,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, @@ -1214,7 +1336,7 @@ abstract contract SpokePool is function _verifyUpdateV3DepositMessage( address depositor, - uint32 depositId, + uint256 depositId, uint256 originChainId, uint256 updatedOutputAmount, address updatedRecipient, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 0be81c630..a5d09e0e3 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, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 434917ecc..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; @@ -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); 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 53a4860ff27ef7a175246f86ee12aa84953ae5f8 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 9 Oct 2024 02:37:50 -0400 Subject: [PATCH 03/23] add deposit nonces to 7683 Signed-off-by: Matt Rice --- contracts/erc7683/ERC7683Across.sol | 3 +- contracts/erc7683/ERC7683OrderDepositor.sol | 19 +++++-- .../erc7683/ERC7683OrderDepositorExternal.sol | 54 +++++++++++++------ 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/contracts/erc7683/ERC7683Across.sol b/contracts/erc7683/ERC7683Across.sol index 87c29c362..230873c46 100644 --- a/contracts/erc7683/ERC7683Across.sol +++ b/contracts/erc7683/ERC7683Across.sol @@ -13,6 +13,7 @@ struct AcrossOrderData { uint32 destinationChainId; address recipient; address exclusiveRelayer; + uint256 depositNonce; uint32 exclusivityPeriod; bytes message; } @@ -35,7 +36,7 @@ bytes constant ACROSS_ORDER_DATA_TYPE = abi.encodePacked( "address recipient,", "address exclusiveRelayer," "uint32 exclusivityPeriod,", - "bytes message)" + "bytes message,)" ); bytes32 constant ACROSS_ORDER_DATA_TYPE_HASH = keccak256(ACROSS_ORDER_DATA_TYPE); diff --git a/contracts/erc7683/ERC7683OrderDepositor.sol b/contracts/erc7683/ERC7683OrderDepositor.sol index bd5e7e03c..6492ff32a 100644 --- a/contracts/erc7683/ERC7683OrderDepositor.sol +++ b/contracts/erc7683/ERC7683OrderDepositor.sol @@ -70,6 +70,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.outputAmount, acrossOrderData.destinationChainId, acrossOriginFillerData.exclusiveRelayer, + acrossOrderData.depositNonce, // Note: simplifying assumption to avoid quote timestamps that cause orders to expire before the deadline. SafeCast.toUint32(order.openDeadline - QUOTE_BEFORE_DEADLINE), order.fillDeadline, @@ -100,6 +101,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.outputAmount, acrossOrderData.destinationChainId, acrossOrderData.exclusiveRelayer, + acrossOrderData.depositNonce, // Note: simplifying assumption to avoid the order type having to bake in the quote timestamp. SafeCast.toUint32(block.timestamp), order.fillDeadline, @@ -158,6 +160,16 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { return SafeCast.toUint32(block.timestamp); // solhint-disable-line not-rely-on-time } + /** + * @notice Computes the Across depositId for an order with certain params. + * @dev if a 0 depositNonce is used, the depositId will not be deterministic, but you will be safe from collisions. + * See the unsafeDepositV3 method on SpokePool for more details. + * @param depositNonce the depositNonce field in the order. + * @param depositor the sender or signer of the order. + * @return the resulting Across depositId. + */ + function computeDepositId(uint256 depositNonce, address depositor) public view virtual returns (uint256); + function _resolveFor(GaslessCrossChainOrder calldata order, bytes calldata fillerData) internal view @@ -223,7 +235,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.inputAmount, acrossOrderData.outputAmount, block.chainid, - _currentDepositId(), + computeDepositId(acrossOrderData.depositNonce, order.user), order.fillDeadline, acrossOrderData.exclusivityPeriod, acrossOrderData.message @@ -286,7 +298,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.inputAmount, acrossOrderData.outputAmount, block.chainid, - _currentDepositId(), + computeDepositId(acrossOrderData.depositNonce, msg.sender), order.fillDeadline, acrossOrderData.exclusivityPeriod, acrossOrderData.message @@ -347,13 +359,12 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, + uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityDeadline, bytes memory message ) internal virtual; - function _currentDepositId() internal view virtual returns (uint32); - function _destinationSettler(uint256 chainId) internal view virtual returns (address); } diff --git a/contracts/erc7683/ERC7683OrderDepositorExternal.sol b/contracts/erc7683/ERC7683OrderDepositorExternal.sol index 5b34ee74d..adb383404 100644 --- a/contracts/erc7683/ERC7683OrderDepositorExternal.sol +++ b/contracts/erc7683/ERC7683OrderDepositorExternal.sol @@ -50,6 +50,7 @@ contract ERC7683OrderDepositorExternal is ERC7683OrderDepositor, Ownable, MultiC uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, + uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityDeadline, @@ -57,24 +58,45 @@ contract ERC7683OrderDepositorExternal is ERC7683OrderDepositor, Ownable, MultiC ) internal override { IERC20(inputToken).safeIncreaseAllowance(address(SPOKE_POOL), inputAmount); - SPOKE_POOL.depositV3( - depositor, - recipient, - inputToken, - outputToken, - inputAmount, - outputAmount, - destinationChainId, - exclusiveRelayer, - quoteTimestamp, - fillDeadline, - exclusivityDeadline, - message - ); + if (depositNonce == 0) { + SPOKE_POOL.depositV3( + depositor, + recipient, + inputToken, + outputToken, + inputAmount, + outputAmount, + destinationChainId, + exclusiveRelayer, + quoteTimestamp, + fillDeadline, + exclusivityDeadline, + message + ); + } else { + SPOKE_POOL.unsafeDepositV3( + depositor, + recipient, + inputToken, + outputToken, + inputAmount, + outputAmount, + destinationChainId, + exclusiveRelayer, + depositNonce, + quoteTimestamp, + fillDeadline, + exclusivityDeadline, + message + ); + } } - function _currentDepositId() internal view override returns (uint32) { - return SPOKE_POOL.numberOfDeposits(); + function computeDepositId(uint256 depositNonce, address depositor) public view override returns (uint256) { + return + depositNonce == 0 + ? SPOKE_POOL.numberOfDeposits() + : SPOKE_POOL.getUnsafeDepositId(address(this), depositor, depositNonce); } function _destinationSettler(uint256 chainId) internal view override returns (address) { From e12e716e0a6b9ef048c5e6cc535a1d8cf0a23eb9 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 9 Oct 2024 02:57:41 -0400 Subject: [PATCH 04/23] fix Signed-off-by: Matt Rice --- contracts/erc7683/ERC7683Across.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/erc7683/ERC7683Across.sol b/contracts/erc7683/ERC7683Across.sol index 230873c46..642244721 100644 --- a/contracts/erc7683/ERC7683Across.sol +++ b/contracts/erc7683/ERC7683Across.sol @@ -35,8 +35,9 @@ bytes constant ACROSS_ORDER_DATA_TYPE = abi.encodePacked( "uint32 destinationChainId,", "address recipient,", "address exclusiveRelayer," + "uint256 depositNonce,", "uint32 exclusivityPeriod,", - "bytes message,)" + "bytes message)" ); bytes32 constant ACROSS_ORDER_DATA_TYPE_HASH = keccak256(ACROSS_ORDER_DATA_TYPE); From aae704846efca2e3095b285204f042ed3527e5d7 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Wed, 9 Oct 2024 03:01:13 -0400 Subject: [PATCH 05/23] WIP Signed-off-by: Matt Rice --- contracts/erc7683/ERC7683OrderDepositor.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contracts/erc7683/ERC7683OrderDepositor.sol b/contracts/erc7683/ERC7683OrderDepositor.sol index 6492ff32a..06857c69d 100644 --- a/contracts/erc7683/ERC7683OrderDepositor.sol +++ b/contracts/erc7683/ERC7683OrderDepositor.sol @@ -161,9 +161,10 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { } /** - * @notice Computes the Across depositId for an order with certain params. - * @dev if a 0 depositNonce is used, the depositId will not be deterministic, but you will be safe from collisions. - * See the unsafeDepositV3 method on SpokePool for more details. + * @notice Convenience method to compute the Across depositId for orders sent through 7683. + * @dev if a 0 depositNonce is used, the depositId will not be deterministic (meaning it can change depending on + * when the open txn is mined), but you will be safe from collisions. See the unsafeDepositV3 method on SpokePool + * for more details on how to choose between deterministic and non-deterministic. * @param depositNonce the depositNonce field in the order. * @param depositor the sender or signer of the order. * @return the resulting Across depositId. From 7641fbf38b661e02fc00f3b33d7e587c6dc5f06f Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Tue, 8 Oct 2024 09:55:17 -0400 Subject: [PATCH 06/23] feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583) --- contracts/SpokePool.sol | 210 ++++++++++++++---- contracts/interfaces/V3SpokePoolInterface.sol | 12 +- test/evm/hardhat/SpokePool.Deposit.ts | 62 +++++- .../evm/hardhat/fixtures/SpokePool.Fixture.ts | 2 +- 4 files changed, 234 insertions(+), 52 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index a9720518a..8090c4329 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -127,7 +127,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. @@ -551,59 +551,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 ); } @@ -757,7 +790,7 @@ abstract contract SpokePool is */ function speedUpV3Deposit( address depositor, - uint32 depositId, + uint256 depositId, uint256 updatedOutputAmount, address updatedRecipient, bytes calldata updatedMessage, @@ -1093,10 +1126,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, @@ -1223,7 +1345,7 @@ abstract contract SpokePool is function _verifyUpdateV3DepositMessage( address depositor, - uint32 depositId, + uint256 depositId, uint256 originChainId, uint256 updatedOutputAmount, address updatedRecipient, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 0be81c630..a5d09e0e3 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, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 434917ecc..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; @@ -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); 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 7423b9ecaaf7d558ea8d6d0c46976717f3997fa2 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 9 Oct 2024 23:58:38 +0200 Subject: [PATCH 07/23] fix(SpokePool): Apply exclusivity consistently The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit(). Identified via test case failures in the relayer. Signed-off-by: Paul <108695806+pxrl@users.noreply.github.com> --- contracts/SpokePool.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index a9720518a..76bba42d6 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -885,7 +885,11 @@ abstract contract SpokePool is ) public override nonReentrant unpausedFills { // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. - if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != msg.sender) { + if ( + relayData.exclusiveRelayer != msg.sender && + relayData.exclusivityDeadline >= getCurrentTime() && + relayData.exclusiveRelayer != address(0) + ) { revert NotExclusiveRelayer(); } From 9e8e05744a6c81cd6a211c31f50fac25da00427b Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Thu, 10 Oct 2024 00:12:10 +0200 Subject: [PATCH 08/23] Also check on slow fill requests --- contracts/SpokePool.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 76bba42d6..692892581 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -935,7 +935,13 @@ abstract contract SpokePool is // fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested // during this exclusivity period. - if (relayData.exclusivityDeadline >= getCurrentTime()) revert NoSlowFillsInExclusivityWindow(); + if ( + relayData.exclusiveRelayer != msg.sender && + relayData.exclusivityDeadline >= getCurrentTime() && + relayData.exclusiveRelayer != address(0) + ) { + revert NotExclusiveRelayer(); + } if (relayData.fillDeadline < getCurrentTime()) revert ExpiredFillDeadline(); bytes32 relayHash = _getV3RelayHash(relayData); From 90b76cd5960d43950ff17465e4dc333dbfce0b35 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Thu, 10 Oct 2024 00:15:53 +0200 Subject: [PATCH 09/23] Update contracts/SpokePool.sol --- contracts/SpokePool.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 692892581..119a51c87 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -936,7 +936,6 @@ abstract contract SpokePool is // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested // during this exclusivity period. if ( - relayData.exclusiveRelayer != msg.sender && relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != address(0) ) { From 014f879d72ef7131a2ed18f1fe5653d11dab0bee Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Thu, 10 Oct 2024 00:34:39 +0200 Subject: [PATCH 10/23] lint --- contracts/SpokePool.sol | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 119a51c87..ec02eb6f8 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -935,10 +935,7 @@ abstract contract SpokePool is // fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested // during this exclusivity period. - if ( - relayData.exclusivityDeadline >= getCurrentTime() && - relayData.exclusiveRelayer != address(0) - ) { + if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != address(0)) { revert NotExclusiveRelayer(); } if (relayData.fillDeadline < getCurrentTime()) revert ExpiredFillDeadline(); From 0b6565946d06dc8c5963ed28d72ab22494704269 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 11 Oct 2024 12:20:36 +0000 Subject: [PATCH 11/23] Update --- contracts/SpokePool.sol | 17 ++++++++++------- contracts/interfaces/V3SpokePoolInterface.sol | 1 - 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index ec02eb6f8..afa48414d 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -839,9 +839,8 @@ abstract contract SpokePool is // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. if ( - relayData.exclusiveRelayer != msg.sender && - relayData.exclusivityDeadline >= getCurrentTime() && - relayData.exclusiveRelayer != address(0) + _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline) && + relayData.exclusiveRelayer != msg.sender ) { revert NotExclusiveRelayer(); } @@ -886,9 +885,8 @@ abstract contract SpokePool is // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. if ( - relayData.exclusiveRelayer != msg.sender && - relayData.exclusivityDeadline >= getCurrentTime() && - relayData.exclusiveRelayer != address(0) + _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline) && + relayData.exclusiveRelayer != msg.sender ) { revert NotExclusiveRelayer(); } @@ -935,7 +933,7 @@ abstract contract SpokePool is // fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested // during this exclusivity period. - if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != address(0)) { + if (_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline)) { revert NotExclusiveRelayer(); } if (relayData.fillDeadline < getCurrentTime()) revert ExpiredFillDeadline(); @@ -1414,6 +1412,11 @@ abstract contract SpokePool is } } + // Determine whether the combination of exlcusiveRelayer and exclusivityDeadline implies active exclusivity. + function _fillIsExclusive(address exclusiveRelayer, uint32 exclusivityDeadline) internal returns (bool) { + return exclusivityDeadline >= getCurrentTime() && exclusiveRelayer != address(0); + } + // Implementing contract needs to override this to ensure that only the appropriate cross chain admin can execute // certain admin functions. For L2 contracts, the cross chain admin refers to some L1 address or contract, and for // L1, this would just be the same admin of the HubPool. diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 0be81c630..d004fdfcc 100644 --- a/contracts/interfaces/V3SpokePoolInterface.sol +++ b/contracts/interfaces/V3SpokePoolInterface.sol @@ -222,7 +222,6 @@ interface V3SpokePoolInterface { error DisabledRoute(); error InvalidQuoteTimestamp(); error InvalidFillDeadline(); - error InvalidExclusiveRelayer(); error InvalidExclusivityDeadline(); error MsgValueDoesNotMatchInputAmount(); error NotExclusiveRelayer(); From 09b298e9921df6694e7ca1a1a4e11d1f2b290538 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 11 Oct 2024 12:29:21 +0000 Subject: [PATCH 12/23] Add pure --- contracts/SpokePool.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index afa48414d..0170bfdd1 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1413,7 +1413,7 @@ abstract contract SpokePool is } // Determine whether the combination of exlcusiveRelayer and exclusivityDeadline implies active exclusivity. - function _fillIsExclusive(address exclusiveRelayer, uint32 exclusivityDeadline) internal returns (bool) { + function _fillIsExclusive(address exclusiveRelayer, uint32 exclusivityDeadline) internal pure returns (bool) { return exclusivityDeadline >= getCurrentTime() && exclusiveRelayer != address(0); } From c24c72306f55764b84beb0434a1d3c7953fe5536 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 11 Oct 2024 12:45:35 +0000 Subject: [PATCH 13/23] Fix --- contracts/SpokePool.sol | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 0170bfdd1..fb5c320e6 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -839,7 +839,7 @@ abstract contract SpokePool is // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. if ( - _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline) && + _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) && relayData.exclusiveRelayer != msg.sender ) { revert NotExclusiveRelayer(); @@ -885,7 +885,7 @@ abstract contract SpokePool is // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. if ( - _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline) && + _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) && relayData.exclusiveRelayer != msg.sender ) { revert NotExclusiveRelayer(); @@ -929,14 +929,15 @@ abstract contract SpokePool is * then Across will not include a slow fill for the intended deposit. */ function requestV3SlowFill(V3RelayData calldata relayData) public override nonReentrant unpausedFills { + uint32 currentTime = uint32(getCurrentTime()); // If a depositor has set an exclusivity deadline, then only the exclusive relayer should be able to // fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested // during this exclusivity period. - if (_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline)) { - revert NotExclusiveRelayer(); + if (_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, currentTime)) { + revert NoSlowFillsInExclusivityWindow(); } - if (relayData.fillDeadline < getCurrentTime()) revert ExpiredFillDeadline(); + if (relayData.fillDeadline < currentTime) revert ExpiredFillDeadline(); bytes32 relayHash = _getV3RelayHash(relayData); if (fillStatuses[relayHash] != uint256(FillStatus.Unfilled)) revert InvalidSlowFillRequest(); @@ -1413,8 +1414,12 @@ abstract contract SpokePool is } // Determine whether the combination of exlcusiveRelayer and exclusivityDeadline implies active exclusivity. - function _fillIsExclusive(address exclusiveRelayer, uint32 exclusivityDeadline) internal pure returns (bool) { - return exclusivityDeadline >= getCurrentTime() && exclusiveRelayer != address(0); + function _fillIsExclusive( + address exclusiveRelayer, + uint32 exclusivityDeadline, + uint32 currentTime + ) internal pure returns (bool) { + return exclusivityDeadline >= currentTime && exclusiveRelayer != address(0); } // Implementing contract needs to override this to ensure that only the appropriate cross chain admin can execute From 6b5f5f9f65a927bb272b59e1a5fe481b22c06742 Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Fri, 11 Oct 2024 22:22:37 +0000 Subject: [PATCH 14/23] Add tests --- contracts/SpokePool.sol | 2 +- test/evm/hardhat/SpokePool.Relay.ts | 17 +++++++++++++++++ test/evm/hardhat/SpokePool.SlowRelay.ts | 7 +++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index fb5c320e6..10b9d8ed3 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -319,7 +319,7 @@ abstract contract SpokePool is /** * @notice Pauses deposit-related functions. This is intended to be used if this contract is deprecated or when * something goes awry. - * @dev Affects `deposit()` but not `speedUpDeposit()`, so that existing deposits can be sped up and still + * @dev Affects `deposit()` but not `speedUpV3Deposit()`, so that existing deposits can be sped up and still * relayed. * @param pause true if the call is meant to pause the system, false if the call is meant to unpause it. */ diff --git a/test/evm/hardhat/SpokePool.Relay.ts b/test/evm/hardhat/SpokePool.Relay.ts index ca35500d9..88c0aeaf3 100644 --- a/test/evm/hardhat/SpokePool.Relay.ts +++ b/test/evm/hardhat/SpokePool.Relay.ts @@ -413,6 +413,23 @@ describe("SpokePool Relayer Logic", async function () { updatedMessage ); }); + it("in absence of exclusivity", async function () { + // Clock drift between spokes can mean exclusivityDeadline is in future even when no exclusivity was applied. + await spokePool.setCurrentTime(relayData.exclusivityDeadline - 1); + await expect( + spokePool.connect(relayer).fillV3RelayWithUpdatedDeposit( + { + ...relayData, + exclusiveRelayer: consts.zeroAddress, + }, + consts.repaymentChainId, + updatedOutputAmount, + updatedRecipient, + updatedMessage, + signature + ) + ).to.emit(spokePool, "FilledV3Relay"); + }); it("must be exclusive relayer before exclusivity deadline", async function () { const _relayData = { ...relayData, diff --git a/test/evm/hardhat/SpokePool.SlowRelay.ts b/test/evm/hardhat/SpokePool.SlowRelay.ts index a8da51d20..4a0ded716 100644 --- a/test/evm/hardhat/SpokePool.SlowRelay.ts +++ b/test/evm/hardhat/SpokePool.SlowRelay.ts @@ -51,6 +51,13 @@ describe("SpokePool Slow Relay Logic", async function () { relayData.fillDeadline = (await spokePool.getCurrentTime()).sub(1); await expect(spokePool.connect(relayer).requestV3SlowFill(relayData)).to.be.revertedWith("ExpiredFillDeadline"); }); + it("in absence of exclusivity", async function () { + // Clock drift between spokes can mean exclusivityDeadline is in future even when no exclusivity was applied. + await spokePool.setCurrentTime(relayData.exclusivityDeadline - 1); + await expect( + spokePool.connect(relayer).requestV3SlowFill({ ...relayData, exclusiveRelayer: consts.zeroAddress }) + ).to.emit(spokePool, "RequestedV3SlowFill"); + }); it("during exclusivity deadline", async function () { await spokePool.setCurrentTime(relayData.exclusivityDeadline); await expect(spokePool.connect(relayer).requestV3SlowFill(relayData)).to.be.revertedWith( From 33c6d8e8f912b9773d5d707a9920e46fd8c6536a Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 17 Oct 2024 17:05:11 -0400 Subject: [PATCH 15/23] improve(SpokePool): _depositV3 interprets `exclusivityParameter` as 0, an offset, or a timestamp There should be a way for the deposit transaction to remove chain re-org risk affecting the block.timestamp by allowing the caller to set a fixed `exclusivityDeadline` value. This supports the existing behavior where the `exclusivityDeadline` is always emitted as its passed in. The new behavior is that if the `exclusivityParameter`, which replaces the `exclusivityDeadlineOffset` parameter, is 0 or greater than 1 year in seconds, then the `exclusivityDeadline` is equal to this parameter. Otherwise, its interpreted by `_depositV3()` as an offset. The offset would be useful in cases where the origin chain will not re-org, for example. --- contracts/SpokePool.sol | 71 +++++-- contracts/interfaces/V3SpokePoolInterface.sol | 1 - test/evm/hardhat/SpokePool.Deposit.ts | 177 +++++++++++++++++- test/evm/hardhat/SpokePool.Relay.ts | 20 -- test/evm/hardhat/constants.ts | 2 + 5 files changed, 225 insertions(+), 46 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 8090c4329..87e329826 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -143,6 +143,10 @@ abstract contract SpokePool is // token for the input token. By using this magic value, off-chain validators do not have to keep // this event in their lookback window when querying for expired deposts. uint32 public constant INFINITE_FILL_DEADLINE = type(uint32).max; + + // One year in seconds. If `exclusivityParameter` is set to a value less than this, then the emitted + // exclusivityDeadline in a deposit event will be set to the current time plus this value. + uint32 public constant MAX_EXCLUSIVITY_PERIOD_SECONDS = 31_536_000; /**************************************** * EVENTS * ****************************************/ @@ -490,9 +494,8 @@ abstract contract SpokePool is /** * @notice Previously, this function allowed the caller to specify the exclusivityDeadline, otherwise known as the * as exact timestamp on the destination chain before which only the exclusiveRelayer could fill the deposit. Now, - * the caller is expected to pass in an exclusivityPeriod which is the number of seconds to be added to the - * block.timestamp to produce the exclusivityDeadline. This allows the caller to ignore any latency associated - * with this transaction being mined and propagating this transaction to the miner. + * the caller is expected to pass in a number that will be interpreted either as an offset or a fixed + * timestamp depending on its value. * @notice Request to bridge input token cross chain to a destination chain and receive a specified amount * of output tokens. The fee paid to relayers and the system should be captured in the spread between output * amount and input amount when adjusted to be denominated in the input token. A relayer on the destination @@ -531,9 +534,17 @@ abstract contract SpokePool is * @param fillDeadline The deadline for the relayer to fill the deposit. After this destination chain timestamp, * the fill will revert on the destination chain. Must be set between [currentTime, currentTime + fillDeadlineBuffer] * where currentTime is block.timestamp on this chain or this transaction will revert. - * @param exclusivityPeriod Added to the current time to set the exclusive relayer deadline, - * which is the deadline for the exclusiveRelayer to fill the deposit. After this destination chain timestamp, - * anyone can fill the deposit. + * @param exclusivityParameter This value is used to set the exclusivity deadline timestamp in the emitted deposit + * event. Before this destinationchain timestamp, only the exclusiveRelayer (if set to a non-zero address), + * can fill this deposit. There are three ways to use this parameter: + * 1. NO EXCLUSIVITY: If this value is set to 0, then a timestamp of 0 will be emitted, + * meaning that there is no exclusivity period. + * 2. OFFSET: If this value is less than MAX_EXCLUSIVITY_PERIOD_SECONDS, then add this value to + * the block.timestamp to derive the exclusive relayer deadline. Note that using the parameter in this way + * will expose the filler of the deposit to the risk that the block.timestamp of this event gets changed + * due to a chain-reorg, which would also change the exclusivity timestamp. + * 3. TIMESTAMP: Otherwise, set this value as the exclusivity deadline timestamp. + * which is the deadline for the exclusiveRelayer to fill the deposit. * @param message The message to send to the recipient on the destination chain if the recipient is a contract. * If the message is not empty, the recipient contract must implement handleV3AcrossMessage() or the fill will revert. */ @@ -548,7 +559,7 @@ abstract contract SpokePool is address exclusiveRelayer, uint32 quoteTimestamp, uint32 fillDeadline, - uint32 exclusivityPeriod, + uint32 exclusivityParameter, bytes calldata message ) public payable override nonReentrant unpausedDeposits { _depositV3( @@ -566,7 +577,7 @@ abstract contract SpokePool is numberOfDeposits++, quoteTimestamp, fillDeadline, - uint32(getCurrentTime()) + exclusivityPeriod, + exclusivityParameter, message ); } @@ -600,7 +611,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 exclusivityPeriod See identically named parameter in depositV3() comments. + * @param exclusivityParameter See identically named parameter in depositV3() comments. * @param message See identically named parameter in depositV3() comments. */ function unsafeDepositV3( @@ -615,7 +626,7 @@ abstract contract SpokePool is uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, - uint32 exclusivityPeriod, + uint32 exclusivityParameter, bytes calldata message ) public payable nonReentrant unpausedDeposits { // @dev Create the uint256 deposit ID by concatenating the msg.sender and depositor address with the inputted @@ -636,7 +647,7 @@ abstract contract SpokePool is depositId, quoteTimestamp, fillDeadline, - uint32(getCurrentTime()) + exclusivityPeriod, + exclusivityParameter, message ); } @@ -857,7 +868,9 @@ abstract contract SpokePool is * - fillDeadline The deadline for the caller to fill the deposit. After this timestamp, * the fill will revert on the destination chain. * - exclusivityDeadline: The deadline for the exclusive relayer to fill the deposit. After this - * timestamp, anyone can fill this deposit. + * timestamp, anyone can fill this deposit. Note that if this value was set in depositV3 by adding an offset + * to the deposit's block.timestamp, there is re-org risk for the caller of this method because the event's + * block.timestamp can change. Read the comments in `depositV3` about the `exclusivityParameter` for more details. * - message The message to send to the recipient if the recipient is a contract that implements a * handleV3AcrossMessage() public function * @param repaymentChainId Chain of SpokePool where relayer wants to be refunded after the challenge window has @@ -871,11 +884,7 @@ abstract contract SpokePool is { // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. - if ( - relayData.exclusiveRelayer != msg.sender && - relayData.exclusivityDeadline >= getCurrentTime() && - relayData.exclusiveRelayer != address(0) - ) { + if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != msg.sender) { revert NotExclusiveRelayer(); } @@ -1160,7 +1169,7 @@ abstract contract SpokePool is uint256 depositId, uint32 quoteTimestamp, uint32 fillDeadline, - uint32 exclusivityDeadline, + uint32 exclusivityParameter, bytes calldata message ) internal { // Check that deposit route is enabled for the input token. There are no checks required for the output token @@ -1187,6 +1196,32 @@ abstract contract SpokePool is // situation won't be a problem for honest users. if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline(); + // There are three cases for setting the exclusivity deadline using the exclusivity parameter: + // 1. If this parameter is 0, then there is no exclusivity period and emit 0 for the deadline. This + // means that fillers of this deposit do not have to worry about the block.timestamp of this event changing + // due to re-orgs when filling this deposit. + // 2. If the exclusivity parameter is less than or equal to MAX_EXCLUSIVITY_PERIOD_SECONDS, then the exclusivity + // deadline is set to the block.timestamp of this event plus the exclusivity parameter. This means that the + // filler of this deposit assumes re-org risk when filling this deposit because the block.timestamp of this + // event affects the exclusivity deadline. + // 3. Otherwise, interpret this parameter as a timestamp and emit it as the exclusivity deadline. This means + // that the filler of this deposit will not assume re-org risk related to the block.timestamp of this + // event changing. + uint32 exclusivityDeadline; + if (exclusivityParameter == 0) { + exclusivityDeadline = 0; + } else { + if (exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { + exclusivityDeadline = uint32(currentTime) + exclusivityParameter; + } else { + exclusivityDeadline = exclusivityParameter; + } + + // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period + // by forcing them to specify an exclusive relayer. + if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); + } + // 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. diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index a5d09e0e3..326ee43a6 100644 --- a/contracts/interfaces/V3SpokePoolInterface.sol +++ b/contracts/interfaces/V3SpokePoolInterface.sol @@ -223,7 +223,6 @@ interface V3SpokePoolInterface { error InvalidQuoteTimestamp(); error InvalidFillDeadline(); error InvalidExclusiveRelayer(); - error InvalidExclusivityDeadline(); error MsgValueDoesNotMatchInputAmount(); error NotExclusiveRelayer(); error NoSlowFillsInExclusivityWindow(); diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index fd6b7a4e3..5be855d19 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -25,6 +25,8 @@ import { amountReceived, MAX_UINT32, originChainId, + MAX_EXCLUSIVITY_OFFSET_SECONDS, + zeroAddress, } from "./constants"; const { AddressZero: ZERO_ADDRESS } = ethers.constants; @@ -457,6 +459,170 @@ describe("SpokePool Depositor Logic", async function () { ) ).to.not.be.reverted; }); + it("invalid exclusivity params", async function () { + const currentTime = await spokePool.getCurrentTime(); + + // If exclusive deadline is not zero, then exclusive relayer must be set. + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: 1, + }) + ) + ).to.be.revertedWith("InvalidExclusiveRelayer"); + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: MAX_EXCLUSIVITY_OFFSET_SECONDS, + }) + ) + ).to.be.revertedWith("InvalidExclusiveRelayer"); + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: MAX_EXCLUSIVITY_OFFSET_SECONDS + 1, + }) + ) + ).to.be.revertedWith("InvalidExclusiveRelayer"); + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: currentTime.sub(1), + }) + ) + ).to.be.revertedWith("InvalidExclusiveRelayer"); + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: currentTime.add(1), + }) + ) + ).to.be.revertedWith("InvalidExclusiveRelayer"); + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData({ + ...relayData, + exclusiveRelayer: zeroAddress, + exclusivityDeadline: 0, + }) + ) + ).to.not.be.reverted; + }); + it("exclusivity param is used as an offset", async function () { + const currentTime = (await spokePool.getCurrentTime()).toNumber(); + const fillDeadlineOffset = 1000; + const exclusivityDeadlineOffset = MAX_EXCLUSIVITY_OFFSET_SECONDS; + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData( + { + ...relayData, + exclusiveRelayer: depositor.address, + exclusivityDeadline: exclusivityDeadlineOffset, + }, + undefined, + currentTime + ) + ) + ) + .to.emit(spokePool, "V3FundsDeposited") + .withArgs( + relayData.inputToken, + relayData.outputToken, + relayData.inputAmount, + relayData.outputAmount, + destinationChainId, + // deposit ID is 0 for first deposit + 0, + currentTime, // quoteTimestamp should be current time + currentTime + fillDeadlineOffset, // fillDeadline should be current time + offset + currentTime + exclusivityDeadlineOffset, // exclusivityDeadline should be current time + offset + relayData.depositor, + relayData.recipient, + depositor.address, + relayData.message + ); + }); + it("exclusivity param is used as a timestamp", async function () { + const currentTime = (await spokePool.getCurrentTime()).toNumber(); + const fillDeadlineOffset = 1000; + const exclusivityDeadlineTimestamp = MAX_EXCLUSIVITY_OFFSET_SECONDS + 1; + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData( + { + ...relayData, + exclusiveRelayer: depositor.address, + exclusivityDeadline: exclusivityDeadlineTimestamp, + }, + undefined, + currentTime + ) + ) + ) + .to.emit(spokePool, "V3FundsDeposited") + .withArgs( + relayData.inputToken, + relayData.outputToken, + relayData.inputAmount, + relayData.outputAmount, + destinationChainId, + // deposit ID is 0 for first deposit + 0, + currentTime, // quoteTimestamp should be current time + currentTime + fillDeadlineOffset, // fillDeadline should be current time + offset + exclusivityDeadlineTimestamp, // exclusivityDeadline should be passed in time + relayData.depositor, + relayData.recipient, + depositor.address, + relayData.message + ); + }); + it("exclusivity param is set to 0", async function () { + const currentTime = (await spokePool.getCurrentTime()).toNumber(); + const fillDeadlineOffset = 1000; + const zeroExclusivity = 0; + await expect( + spokePool.connect(depositor).depositV3( + ...getDepositArgsFromRelayData( + { + ...relayData, + exclusiveRelayer: depositor.address, + exclusivityDeadline: zeroExclusivity, + }, + undefined, + currentTime + ) + ) + ) + .to.emit(spokePool, "V3FundsDeposited") + .withArgs( + relayData.inputToken, + relayData.outputToken, + relayData.inputAmount, + relayData.outputAmount, + destinationChainId, + // deposit ID is 0 for first deposit + 0, + currentTime, // quoteTimestamp should be current time + currentTime + fillDeadlineOffset, // fillDeadline should be current time + offset + 0, // Exclusivity deadline should always be 0 + relayData.depositor, + relayData.recipient, + depositor.address, + relayData.message + ); + }); it("if input token is WETH and msg.value > 0, msg.value must match inputAmount", async function () { await expect( spokePool @@ -527,7 +693,7 @@ describe("SpokePool Depositor Logic", async function () { 0, currentTime, // quoteTimestamp should be current time currentTime + fillDeadlineOffset, // fillDeadline should be current time + offset - currentTime, + 0, relayData.depositor, relayData.recipient, relayData.exclusiveRelayer, @@ -535,7 +701,6 @@ describe("SpokePool Depositor Logic", async function () { ); }); it("emits V3FundsDeposited event with correct deposit ID", async function () { - const currentTime = (await spokePool.getCurrentTime()).toNumber(); await expect(spokePool.connect(depositor).depositV3(...depositArgs)) .to.emit(spokePool, "V3FundsDeposited") .withArgs( @@ -548,7 +713,7 @@ describe("SpokePool Depositor Logic", async function () { 0, quoteTimestamp, relayData.fillDeadline, - currentTime, + 0, relayData.depositor, relayData.recipient, relayData.exclusiveRelayer, @@ -560,7 +725,6 @@ describe("SpokePool Depositor Logic", async function () { expect(await spokePool.numberOfDeposits()).to.equal(1); }); it("tokens are always pulled from caller, even if different from specified depositor", async function () { - const currentTime = (await spokePool.getCurrentTime()).toNumber(); const balanceBefore = await erc20.balanceOf(depositor.address); const newDepositor = randomAddress(); await expect( @@ -578,7 +742,7 @@ describe("SpokePool Depositor Logic", async function () { 0, quoteTimestamp, relayData.fillDeadline, - currentTime, + 0, // New depositor newDepositor, relayData.recipient, @@ -598,7 +762,6 @@ 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 {msg.sender, depositor, forcedDepositId}. const forcedDepositId = "99"; const expectedDepositId = BigNumber.from( @@ -629,7 +792,7 @@ describe("SpokePool Depositor Logic", async function () { expectedDepositId, quoteTimestamp, relayData.fillDeadline, - currentTime, + 0, recipient.address, relayData.recipient, relayData.exclusiveRelayer, diff --git a/test/evm/hardhat/SpokePool.Relay.ts b/test/evm/hardhat/SpokePool.Relay.ts index ca35500d9..98dd4469e 100644 --- a/test/evm/hardhat/SpokePool.Relay.ts +++ b/test/evm/hardhat/SpokePool.Relay.ts @@ -349,26 +349,6 @@ describe("SpokePool Relayer Logic", async function () { ) ).to.not.be.reverted; }); - it("if no exclusive relayer is set, exclusivity deadline can be in future", async function () { - const _relayData = { - ...relayData, - // Overwrite exclusivity deadline - exclusivityDeadline: relayData.fillDeadline, - }; - - // Can send it after exclusivity deadline - await expect(spokePool.connect(relayer).fillV3Relay(_relayData, consts.repaymentChainId)).to.not.be.reverted; - }); - it("can have empty exclusive relayer before exclusivity deadline", async function () { - const _relayData = { - ...relayData, - // Overwrite exclusivity deadline - exclusivityDeadline: relayData.fillDeadline, - }; - - // Can send it before exclusivity deadline if exclusive relayer is empty - await expect(spokePool.connect(relayer).fillV3Relay(_relayData, consts.repaymentChainId)).to.not.be.reverted; - }); it("calls _fillRelayV3 with expected params", async function () { await expect(spokePool.connect(relayer).fillV3Relay(relayData, consts.repaymentChainId)) .to.emit(spokePool, "FilledV3Relay") diff --git a/test/evm/hardhat/constants.ts b/test/evm/hardhat/constants.ts index cb4bf6de3..14c2f6075 100644 --- a/test/evm/hardhat/constants.ts +++ b/test/evm/hardhat/constants.ts @@ -100,6 +100,8 @@ export const l1TokenTransferThreshold = toWei(100); export const MAX_UINT32 = BigNumber.from("0xFFFFFFFF"); +export const MAX_EXCLUSIVITY_OFFSET_SECONDS = 24 * 60 * 60 * 365; // 1 year + // DAI's Rate model. export const sampleRateModel = { UBar: toWei(0.8).toString(), From 2e3d6715d102e97a47d8a0a2ed4745f22d67a3de Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 22 Oct 2024 15:20:34 -0400 Subject: [PATCH 16/23] Update SpokePool.sol --- contracts/SpokePool.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 7700c12db..a6e77d07f 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -884,7 +884,10 @@ abstract contract SpokePool is { // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right // to fill the relay. - if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != msg.sender) { + if ( + _fillIsExclusive(relayData.exclusivityDeadline, uint32(getCurrentTime())) && + relayData.exclusiveRelayer != msg.sender + ) { revert NotExclusiveRelayer(); } From cf3e9638a88a4ac732a79a9e12a79e12d5ad566c Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 22 Oct 2024 17:05:19 -0400 Subject: [PATCH 17/23] Update SpokePool.Relay.ts --- test/evm/hardhat/SpokePool.Relay.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/evm/hardhat/SpokePool.Relay.ts b/test/evm/hardhat/SpokePool.Relay.ts index 15cea8028..88ca7de91 100644 --- a/test/evm/hardhat/SpokePool.Relay.ts +++ b/test/evm/hardhat/SpokePool.Relay.ts @@ -400,7 +400,7 @@ describe("SpokePool Relayer Logic", async function () { spokePool.connect(relayer).fillV3RelayWithUpdatedDeposit( { ...relayData, - exclusiveRelayer: consts.zeroAddress, + exclusivityDeadline: 0, }, consts.repaymentChainId, updatedOutputAmount, From 789299c503a4fc914e14c9e972f761a3bfbaa95d Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 23 Oct 2024 09:48:55 -0400 Subject: [PATCH 18/23] Update SpokePool.SlowRelay.ts --- test/evm/hardhat/SpokePool.SlowRelay.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/evm/hardhat/SpokePool.SlowRelay.ts b/test/evm/hardhat/SpokePool.SlowRelay.ts index 4a0ded716..c72dcad09 100644 --- a/test/evm/hardhat/SpokePool.SlowRelay.ts +++ b/test/evm/hardhat/SpokePool.SlowRelay.ts @@ -54,9 +54,10 @@ describe("SpokePool Slow Relay Logic", async function () { it("in absence of exclusivity", async function () { // Clock drift between spokes can mean exclusivityDeadline is in future even when no exclusivity was applied. await spokePool.setCurrentTime(relayData.exclusivityDeadline - 1); - await expect( - spokePool.connect(relayer).requestV3SlowFill({ ...relayData, exclusiveRelayer: consts.zeroAddress }) - ).to.emit(spokePool, "RequestedV3SlowFill"); + await expect(spokePool.connect(relayer).requestV3SlowFill({ ...relayData, exclusivityDeadline: 0 })).to.emit( + spokePool, + "RequestedV3SlowFill" + ); }); it("during exclusivity deadline", async function () { await spokePool.setCurrentTime(relayData.exclusivityDeadline); From 56cc357d6ce9e81aee537a5ccf03395fc4462079 Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Tue, 29 Oct 2024 19:57:01 -0400 Subject: [PATCH 19/23] Update contracts/SpokePool.sol Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> --- contracts/SpokePool.sol | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index a6e77d07f..10a083ba4 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1216,19 +1216,9 @@ abstract contract SpokePool is // 3. Otherwise, interpret this parameter as a timestamp and emit it as the exclusivity deadline. This means // that the filler of this deposit will not assume re-org risk related to the block.timestamp of this // event changing. - uint32 exclusivityDeadline; - if (exclusivityParameter == 0) { - exclusivityDeadline = 0; - } else { - if (exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { - exclusivityDeadline = uint32(currentTime) + exclusivityParameter; - } else { - exclusivityDeadline = exclusivityParameter; - } - - // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period - // by forcing them to specify an exclusive relayer. - if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); + uint32 exclusivityDeadline = exclusivityParameter; + if (exclusivityParameter > 0 && exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { + exclusivityDeadline += uint32(currentTime); } // If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the From 7b20c752e1a3a3d45604d6a555f9e9faa346e445 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 29 Oct 2024 20:04:07 -0400 Subject: [PATCH 20/23] Update SpokePool.sol --- contracts/SpokePool.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 10a083ba4..8ef9c5403 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1217,8 +1217,14 @@ abstract contract SpokePool is // that the filler of this deposit will not assume re-org risk related to the block.timestamp of this // event changing. uint32 exclusivityDeadline = exclusivityParameter; - if (exclusivityParameter > 0 && exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { + if (exclusivityParameter > 0) { + if (exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { exclusivityDeadline += uint32(currentTime); + } + + // As a safety measure, prevent caller from inadvertently locking funds during exclusivity period + // by forcing them to specify an exclusive relayer. + if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); } // If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the From 543e4c189e6e165843157630d0602828f579fe4e Mon Sep 17 00:00:00 2001 From: Paul <108695806+pxrl@users.noreply.github.com> Date: Wed, 30 Oct 2024 09:57:36 +0100 Subject: [PATCH 21/23] Update contracts/SpokePool.sol --- contracts/SpokePool.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 8ef9c5403..322902833 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1217,8 +1217,8 @@ abstract contract SpokePool is // that the filler of this deposit will not assume re-org risk related to the block.timestamp of this // event changing. uint32 exclusivityDeadline = exclusivityParameter; - if (exclusivityParameter > 0) { - if (exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { + if (exclusivityDeadline > 0) { + if (exclusivityDeadline <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { exclusivityDeadline += uint32(currentTime); } From 97b7d3b848d2a34c830536b7622f6dd5473f155a Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 30 Oct 2024 14:14:32 -0400 Subject: [PATCH 22/23] rebase --- contracts/SpokePool.sol | 99 +------------------ contracts/erc7683/ERC7683Across.sol | 2 - contracts/erc7683/ERC7683OrderDepositor.sol | 20 +--- .../erc7683/ERC7683OrderDepositorExternal.sol | 54 +++------- contracts/interfaces/V3SpokePoolInterface.sol | 12 +-- test/evm/hardhat/SpokePool.Deposit.ts | 60 ----------- .../evm/hardhat/fixtures/SpokePool.Fixture.ts | 2 +- 7 files changed, 31 insertions(+), 218 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 322902833..41c4cd99a 100644 --- a/contracts/SpokePool.sol +++ b/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. @@ -571,9 +571,6 @@ abstract contract SpokePool is 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, @@ -582,76 +579,6 @@ abstract contract SpokePool is ); } - /** - * @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 exclusivityParameter 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 exclusivityParameter, - 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, - exclusivityParameter, - message - ); - } - /** * @notice Submits deposit and sets quoteTimestamp to current Time. Sets fill and exclusivity * deadlines as offsets added to the current time. This function is designed to be called by users @@ -801,7 +728,7 @@ abstract contract SpokePool is */ function speedUpV3Deposit( address depositor, - uint256 depositId, + uint32 depositId, uint256 updatedOutputAmount, address updatedRecipient, bytes calldata updatedMessage, @@ -1144,24 +1071,6 @@ 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 * **************************************/ @@ -1175,7 +1084,7 @@ abstract contract SpokePool is uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, - uint256 depositId, + uint32 depositId, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityParameter, @@ -1385,7 +1294,7 @@ abstract contract SpokePool is function _verifyUpdateV3DepositMessage( address depositor, - uint256 depositId, + uint32 depositId, uint256 originChainId, uint256 updatedOutputAmount, address updatedRecipient, diff --git a/contracts/erc7683/ERC7683Across.sol b/contracts/erc7683/ERC7683Across.sol index 642244721..87c29c362 100644 --- a/contracts/erc7683/ERC7683Across.sol +++ b/contracts/erc7683/ERC7683Across.sol @@ -13,7 +13,6 @@ struct AcrossOrderData { uint32 destinationChainId; address recipient; address exclusiveRelayer; - uint256 depositNonce; uint32 exclusivityPeriod; bytes message; } @@ -35,7 +34,6 @@ bytes constant ACROSS_ORDER_DATA_TYPE = abi.encodePacked( "uint32 destinationChainId,", "address recipient,", "address exclusiveRelayer," - "uint256 depositNonce,", "uint32 exclusivityPeriod,", "bytes message)" ); diff --git a/contracts/erc7683/ERC7683OrderDepositor.sol b/contracts/erc7683/ERC7683OrderDepositor.sol index 06857c69d..bd5e7e03c 100644 --- a/contracts/erc7683/ERC7683OrderDepositor.sol +++ b/contracts/erc7683/ERC7683OrderDepositor.sol @@ -70,7 +70,6 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.outputAmount, acrossOrderData.destinationChainId, acrossOriginFillerData.exclusiveRelayer, - acrossOrderData.depositNonce, // Note: simplifying assumption to avoid quote timestamps that cause orders to expire before the deadline. SafeCast.toUint32(order.openDeadline - QUOTE_BEFORE_DEADLINE), order.fillDeadline, @@ -101,7 +100,6 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.outputAmount, acrossOrderData.destinationChainId, acrossOrderData.exclusiveRelayer, - acrossOrderData.depositNonce, // Note: simplifying assumption to avoid the order type having to bake in the quote timestamp. SafeCast.toUint32(block.timestamp), order.fillDeadline, @@ -160,17 +158,6 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { return SafeCast.toUint32(block.timestamp); // solhint-disable-line not-rely-on-time } - /** - * @notice Convenience method to compute the Across depositId for orders sent through 7683. - * @dev if a 0 depositNonce is used, the depositId will not be deterministic (meaning it can change depending on - * when the open txn is mined), but you will be safe from collisions. See the unsafeDepositV3 method on SpokePool - * for more details on how to choose between deterministic and non-deterministic. - * @param depositNonce the depositNonce field in the order. - * @param depositor the sender or signer of the order. - * @return the resulting Across depositId. - */ - function computeDepositId(uint256 depositNonce, address depositor) public view virtual returns (uint256); - function _resolveFor(GaslessCrossChainOrder calldata order, bytes calldata fillerData) internal view @@ -236,7 +223,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.inputAmount, acrossOrderData.outputAmount, block.chainid, - computeDepositId(acrossOrderData.depositNonce, order.user), + _currentDepositId(), order.fillDeadline, acrossOrderData.exclusivityPeriod, acrossOrderData.message @@ -299,7 +286,7 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { acrossOrderData.inputAmount, acrossOrderData.outputAmount, block.chainid, - computeDepositId(acrossOrderData.depositNonce, msg.sender), + _currentDepositId(), order.fillDeadline, acrossOrderData.exclusivityPeriod, acrossOrderData.message @@ -360,12 +347,13 @@ abstract contract ERC7683OrderDepositor is IOriginSettler { uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, - uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityDeadline, bytes memory message ) internal virtual; + function _currentDepositId() internal view virtual returns (uint32); + function _destinationSettler(uint256 chainId) internal view virtual returns (address); } diff --git a/contracts/erc7683/ERC7683OrderDepositorExternal.sol b/contracts/erc7683/ERC7683OrderDepositorExternal.sol index adb383404..5b34ee74d 100644 --- a/contracts/erc7683/ERC7683OrderDepositorExternal.sol +++ b/contracts/erc7683/ERC7683OrderDepositorExternal.sol @@ -50,7 +50,6 @@ contract ERC7683OrderDepositorExternal is ERC7683OrderDepositor, Ownable, MultiC uint256 outputAmount, uint256 destinationChainId, address exclusiveRelayer, - uint256 depositNonce, uint32 quoteTimestamp, uint32 fillDeadline, uint32 exclusivityDeadline, @@ -58,45 +57,24 @@ contract ERC7683OrderDepositorExternal is ERC7683OrderDepositor, Ownable, MultiC ) internal override { IERC20(inputToken).safeIncreaseAllowance(address(SPOKE_POOL), inputAmount); - if (depositNonce == 0) { - SPOKE_POOL.depositV3( - depositor, - recipient, - inputToken, - outputToken, - inputAmount, - outputAmount, - destinationChainId, - exclusiveRelayer, - quoteTimestamp, - fillDeadline, - exclusivityDeadline, - message - ); - } else { - SPOKE_POOL.unsafeDepositV3( - depositor, - recipient, - inputToken, - outputToken, - inputAmount, - outputAmount, - destinationChainId, - exclusiveRelayer, - depositNonce, - quoteTimestamp, - fillDeadline, - exclusivityDeadline, - message - ); - } + SPOKE_POOL.depositV3( + depositor, + recipient, + inputToken, + outputToken, + inputAmount, + outputAmount, + destinationChainId, + exclusiveRelayer, + quoteTimestamp, + fillDeadline, + exclusivityDeadline, + message + ); } - function computeDepositId(uint256 depositNonce, address depositor) public view override returns (uint256) { - return - depositNonce == 0 - ? SPOKE_POOL.numberOfDeposits() - : SPOKE_POOL.getUnsafeDepositId(address(this), depositor, depositNonce); + function _currentDepositId() internal view override returns (uint32) { + return SPOKE_POOL.numberOfDeposits(); } function _destinationSettler(uint256 chainId) internal view override returns (address) { diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 326ee43a6..dd490cd1b 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. - 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, diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index 5be855d19..2f6310db6 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -367,28 +367,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, @@ -761,44 +739,6 @@ describe("SpokePool Depositor Logic", async function () { "ReentrancyGuard: reentrant call" ); }); - it("unsafe deposit ID", async function () { - // 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, - 0, - recipient.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 393cba333..ac5d1d78d 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: "uint256" }, + { name: "depositId", type: "uint32" }, { name: "originChainId", type: "uint256" }, { name: "updatedOutputAmount", type: "uint256" }, { name: "updatedRecipient", type: "address" }, From dddfaee43e8e62f34d2099b6539f3e9a42902d02 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 30 Oct 2024 14:16:56 -0400 Subject: [PATCH 23/23] Update SpokePool.sol --- contracts/SpokePool.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 41c4cd99a..e62d9365f 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -571,6 +571,7 @@ abstract contract SpokePool is outputAmount, destinationChainId, exclusiveRelayer, + // Increment count of deposits so that deposit ID for this spoke pool is unique. numberOfDeposits++, quoteTimestamp, fillDeadline,