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