From e05964b074e6906e4dcb1d0fcd333dc7eb0b87be Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Thu, 5 Jun 2025 01:35:15 -0400 Subject: [PATCH 1/3] feat: disallow deposits with output token set to 0x0 Signed-off-by: Matt Rice --- contracts/SpokePool.sol | 152 +--------- contracts/interfaces/SpokePoolInterface.sol | 23 -- contracts/interfaces/V3SpokePoolInterface.sol | 1 + .../local/SpokePoolDeprecatedMethods.t.sol | 141 --------- test/evm/hardhat/SpokePool.Deposit.ts | 278 +----------------- 5 files changed, 14 insertions(+), 581 deletions(-) delete mode 100644 test/evm/foundry/local/SpokePoolDeprecatedMethods.t.sol diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index c740e3258..71a96aa8e 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -342,90 +342,6 @@ abstract contract SpokePool is emit EmergencyDeletedRootBundle(rootBundleId); } - /************************************** - * LEGACY DEPOSITOR FUNCTIONS * - **************************************/ - - /** - * @dev DEPRECATION NOTICE: this function is deprecated and will be removed in the future. - * Please use deposit (under DEPOSITOR FUNCTIONS below) or depositV3 instead. - * @notice Called by user to bridge funds from origin to destination chain. Depositor will effectively lock - * tokens in this contract and receive a destination token on the destination chain. The origin => destination - * token mapping is stored on the L1 HubPool. - * @notice The caller must first approve this contract to spend amount of originToken. - * @notice The originToken => destinationChainId must be enabled. - * @notice This method is payable because the caller is able to deposit native token if the originToken is - * wrappedNativeToken and this function will handle wrapping the native token to wrappedNativeToken. - * @dev Produces a FundsDeposited event with an infinite expiry, meaning that this deposit can never expire. - * Moreover, the event's outputToken is set to 0x0 meaning that this deposit can always be slow filled. - * @param recipient Address to receive funds at on destination chain. - * @param originToken Token to lock into this contract to initiate deposit. - * @param amount Amount of tokens to deposit. Will be amount of tokens to receive less fees. - * @param destinationChainId Denotes network where user will receive funds from SpokePool by a relayer. - * @param relayerFeePct % of deposit amount taken out to incentivize a fast relayer. - * @param quoteTimestamp Timestamp used by relayers to compute this deposit's realizedLPFeePct which is paid - * to LP pool on HubPool. - * @param message Arbitrary data that can be used to pass additional information to the recipient along with the tokens. - * Note: this is intended to be used to pass along instructions for how a contract should use or allocate the tokens. - */ - function depositDeprecated_5947912356( - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message, - uint256 // maxCount. Deprecated. - ) public payable nonReentrant unpausedDeposits { - _deposit( - msg.sender, - recipient, - originToken, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - message - ); - } - - /** - * @dev DEPRECATION NOTICE: this function is deprecated and will be removed in the future. - * Please use the other deposit or depositV3 instead. - * @notice The only difference between depositFor and deposit is that the depositor address stored - * in the relay hash can be overridden by the caller. This means that the passed in depositor - * can speed up the deposit, which is useful if the deposit is taken from the end user to a middle layer - * contract, like an aggregator or the SpokePoolVerifier, before calling deposit on this contract. - * @notice The caller must first approve this contract to spend amount of originToken. - * @notice The originToken => destinationChainId must be enabled. - * @notice This method is payable because the caller is able to deposit native token if the originToken is - * wrappedNativeToken and this function will handle wrapping the native token to wrappedNativeToken. - * @param depositor Address who is credited for depositing funds on origin chain and can speed up the deposit. - * @param recipient Address to receive funds at on destination chain. - * @param originToken Token to lock into this contract to initiate deposit. - * @param amount Amount of tokens to deposit. Will be amount of tokens to receive less fees. - * @param destinationChainId Denotes network where user will receive funds from SpokePool by a relayer. - * @param relayerFeePct % of deposit amount taken out to incentivize a fast relayer. - * @param quoteTimestamp Timestamp used by relayers to compute this deposit's realizedLPFeePct which is paid - * to LP pool on HubPool. - * @param message Arbitrary data that can be used to pass additional information to the recipient along with the tokens. - * Note: this is intended to be used to pass along instructions for how a contract should use or allocate the tokens. - */ - function depositFor( - address depositor, - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message, - uint256 // maxCount. Deprecated. - ) public payable nonReentrant unpausedDeposits { - _deposit(depositor, recipient, originToken, amount, destinationChainId, relayerFeePct, quoteTimestamp, message); - } - /******************************************** * DEPOSITOR FUNCTIONS * ********************************************/ @@ -1294,6 +1210,9 @@ abstract contract SpokePool is // Verify depositor is a valid EVM address. params.depositor.checkAddress(); + // Verify output token is not zero address. + if (params.outputToken == bytes32(0)) revert InvalidOutputToken(); + // 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 @@ -1369,71 +1288,6 @@ abstract contract SpokePool is ); } - function _deposit( - address depositor, - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message - ) internal { - // We limit the relay fees to prevent the user spending all their funds on fees. - if (SignedMath.abs(relayerFeePct) >= 0.5e18) revert InvalidRelayerFeePct(); - if (amount > MAX_TRANSFER_SIZE) revert MaxTransferSizeExceeded(); - - // 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 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 - if (getCurrentTime() - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp(); - - // Increment count of deposits so that deposit ID for this spoke pool is unique. - uint32 newDepositId = numberOfDeposits++; - - // 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 ETH. In this case, the ETH should be deposited to wrappedNativeToken. - if (originToken == address(wrappedNativeToken) && msg.value > 0) { - if (msg.value != amount) revert MsgValueDoesNotMatchInputAmount(); - wrappedNativeToken.deposit{ value: msg.value }(); - // Else, it is a normal ERC20. In this case pull the token from the user's wallet as per normal. - // Note: this includes the case where the L2 user 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 { - IERC20Upgradeable(originToken).safeTransferFrom(msg.sender, address(this), amount); - } - - emit FundsDeposited( - originToken.toBytes32(), // inputToken - bytes32(0), // outputToken. Setting this to 0x0 means that the outputToken should be assumed to be the - // canonical token for the destination chain matching the inputToken. Therefore, this deposit - // can always be slow filled. - // - setting token to 0x0 will signal to off-chain validator that the "equivalent" - // token as the inputToken for the destination chain should be replaced here. - amount, // inputAmount - _computeAmountPostFees(amount, relayerFeePct), // outputAmount - // - output amount will be the deposit amount less relayerFeePct, which should now be set - // equal to realizedLpFeePct + gasFeePct + capitalCostFeePct where (gasFeePct + capitalCostFeePct) - // is equal to the old usage of `relayerFeePct`. - destinationChainId, - newDepositId, - quoteTimestamp, - INFINITE_FILL_DEADLINE, // fillDeadline. Default to infinite expiry because - // expired deposits refunds could be a breaking change for existing users of this function. - 0, // exclusivityDeadline. Setting this to 0 along with the exclusiveRelayer to 0x0 means that there - // is no exclusive deadline - depositor.toBytes32(), - recipient.toBytes32(), - bytes32(0), // exclusiveRelayer. Setting this to 0x0 will signal to off-chain validator that there - // is no exclusive relayer. - message - ); - } - function _distributeRelayerRefunds( uint256 _chainId, uint256 amountToReturn, diff --git a/contracts/interfaces/SpokePoolInterface.sol b/contracts/interfaces/SpokePoolInterface.sol index d8ce51cc0..207708b31 100644 --- a/contracts/interfaces/SpokePoolInterface.sol +++ b/contracts/interfaces/SpokePoolInterface.sol @@ -46,29 +46,6 @@ interface SpokePoolInterface { function emergencyDeleteRootBundle(uint256 rootBundleId) external; - function depositDeprecated_5947912356( - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message, - uint256 maxCount - ) external payable; - - function depositFor( - address depositor, - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message, - uint256 maxCount - ) external payable; - function executeRelayerRefundLeaf( uint32 rootBundleId, SpokePoolInterface.RelayerRefundLeaf memory relayerRefundLeaf, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index ea19237c3..249ceabb2 100644 --- a/contracts/interfaces/V3SpokePoolInterface.sol +++ b/contracts/interfaces/V3SpokePoolInterface.sol @@ -327,6 +327,7 @@ interface V3SpokePoolInterface { error InvalidQuoteTimestamp(); error InvalidFillDeadline(); error InvalidExclusiveRelayer(); + error InvalidOutputToken(); error MsgValueDoesNotMatchInputAmount(); error NotExclusiveRelayer(); error NoSlowFillsInExclusivityWindow(); diff --git a/test/evm/foundry/local/SpokePoolDeprecatedMethods.t.sol b/test/evm/foundry/local/SpokePoolDeprecatedMethods.t.sol deleted file mode 100644 index f47ed02b0..000000000 --- a/test/evm/foundry/local/SpokePoolDeprecatedMethods.t.sol +++ /dev/null @@ -1,141 +0,0 @@ -// SPDX-License-Identifier: BUSL-1.1 -pragma solidity ^0.8.0; - -import { Test } from "forge-std/Test.sol"; -import { MockSpokePool } from "../../../../contracts/test/MockSpokePool.sol"; -import { WETH9 } from "../../../../contracts/external/WETH9.sol"; -import { AddressToBytes32 } from "../../../../contracts/libraries/AddressConverters.sol"; -import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; - -// Deprecated interface used to show that we can still call deposit() on the spoke, which should route internally to the -// colliding function interface selector on depositDeprecated_5947912356 enabling legacy deposits to still work without -// breaking interface changes. -interface DeprecatedSpokePoolInterface { - function deposit( - address recipient, - address originToken, - uint256 amount, - uint256 destinationChainId, - int64 relayerFeePct, - uint32 quoteTimestamp, - bytes memory message, - uint256 - ) external payable; -} - -contract SpokePoolOverloadedDeprecatedMethodsTest is Test { - using AddressToBytes32 for address; - - MockSpokePool spokePool; - WETH9 mockWETH; - - address depositor; - address owner; - - uint256 destinationChainId = 10; - uint256 depositAmount = 0.5 * (10**18); - - function setUp() public { - mockWETH = new WETH9(); - - depositor = vm.addr(1); - owner = vm.addr(2); - - vm.startPrank(owner); - ERC1967Proxy proxy = new ERC1967Proxy( - address(new MockSpokePool(address(mockWETH))), - abi.encodeCall(MockSpokePool.initialize, (0, owner, address(420))) - ); - spokePool = MockSpokePool(payable(proxy)); - - vm.stopPrank(); - - deal(depositor, depositAmount * 2); - - vm.startPrank(depositor); - mockWETH.deposit{ value: depositAmount }(); - mockWETH.approve(address(spokePool), depositAmount); - vm.stopPrank(); - - // Assert that the spokePool balance is zero at the start - assertEq(mockWETH.balanceOf(address(spokePool)), 0, "SpokePool balance should be zero at setup"); - } - - function testDeprecatedDeposit() public { - // Here, we are calling the deprecated deposit method, as defined in the deprecated interface. This should, in - // theory, collide with the function selector depositDeprecated_5947912356, thereby calling the legacy deposit - // method on the spoke pool, while using the old old deposit function signature. - vm.startPrank(depositor); - - DeprecatedSpokePoolInterface(address(spokePool)).deposit( - depositor, // recipient - address(mockWETH), // originToken - depositAmount, // amount - destinationChainId, // destinationChainId - 0, // relayerFeePct - uint32(block.timestamp), // quoteTimestamp - bytes(""), // message - 0 // maxCount - ); - - assertEq(mockWETH.balanceOf(address(spokePool)), depositAmount, "SpokePool balance should increase"); - - // Test depositing native ETH directly - DeprecatedSpokePoolInterface(address(spokePool)).deposit{ value: depositAmount }( - depositor, // recipient - address(mockWETH), // originToken - still WETH address for native deposits - depositAmount, // amount - destinationChainId, // destinationChainId - 0, // relayerFeePct - uint32(block.timestamp), // quoteTimestamp - bytes(""), // message - 0 // maxCount - ); - vm.stopPrank(); - - assertEq(mockWETH.balanceOf(address(spokePool)), depositAmount * 2, "SpokePool balance should increase"); - } - - function testBytes32Deposit() public { - vm.prank(depositor); - - // Show the bytes32 variant of the new deposit method works. - spokePool.deposit( - address(depositor).toBytes32(), // depositor - address(depositor).toBytes32(), // recipient - address(mockWETH).toBytes32(), // inputToken - address(mockWETH).toBytes32(), // outputToken - depositAmount, // inputAmount - 0, // outputAmount - destinationChainId, // destinationChainId - bytes32(0), // exclusiveRelayer - uint32(block.timestamp), // quoteTimestamp - uint32(block.timestamp + 1 hours), // fillDeadline - 0, // exclusivityParameter - bytes("") // message - ); - - assertEq(mockWETH.balanceOf(address(spokePool)), depositAmount, "SpokePool balance should increase"); - } - - function testAddressDeposit() public { - // Show the address variant of the new deposit method works. - vm.prank(depositor); - spokePool.depositV3( - depositor, // depositor - depositor, // recipient - address(mockWETH), // inputToken - address(mockWETH), // outputToken - depositAmount, // inputAmount - 0, // outputAmount - destinationChainId, // destinationChainId - address(0), // exclusiveRelayer - uint32(block.timestamp), // quoteTimestamp - uint32(block.timestamp + 1 hours), // fillDeadline - 0, // exclusivityParameter - bytes("") // message - ); - - assertEq(mockWETH.balanceOf(address(spokePool)), depositAmount, "SpokePool balance should increase"); - } -} diff --git a/test/evm/hardhat/SpokePool.Deposit.ts b/test/evm/hardhat/SpokePool.Deposit.ts index db29ce863..e634ebee5 100644 --- a/test/evm/hardhat/SpokePool.Deposit.ts +++ b/test/evm/hardhat/SpokePool.Deposit.ts @@ -53,274 +53,6 @@ describe("SpokePool Depositor Logic", async function () { quoteTimestamp = (await spokePool.getCurrentTime()).toNumber(); }); - it("Depositing ERC20 tokens correctly pulls tokens and changes contract state", async function () { - const revertReason = "DepositsArePaused"; - - // Can't deposit when paused: - await spokePool.connect(depositor).pauseDeposits(true); - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount: amountToDeposit, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }) - ) - ).to.be.revertedWith(revertReason); - - await spokePool.connect(depositor).pauseDeposits(false); - - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - recipient: recipient.address, - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }) - ) - ) - .to.emit(spokePool, "FundsDeposited") - .withArgs( - addressToBytes(erc20.address), - addressToBytes(ZERO_ADDRESS), - amountToDeposit, - amountReceived, - destinationChainId, - 0, - quoteTimestamp, - MAX_UINT32, - 0, - addressToBytes(depositor.address), - addressToBytes(recipient.address), - addressToBytes(ZERO_ADDRESS), - "0x" - ); - - // The collateral should have transferred from depositor to contract. - expect(await erc20.balanceOf(depositor.address)).to.equal(amountToSeedWallets.sub(amountToDeposit)); - expect(await erc20.balanceOf(spokePool.address)).to.equal(amountToDeposit); - - // Deposit nonce should increment. - expect(await spokePool.numberOfDeposits()).to.equal(1); - }); - - it("DepositFor overrrides the depositor", async function () { - const newDepositor = randomAddress(); - await expect( - spokePool.connect(depositor).depositFor( - newDepositor, - ...getDepositParams({ - recipient: recipient.address, - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }) - ) - ) - .to.emit(spokePool, "FundsDeposited") - .withArgs( - addressToBytes(erc20.address), - addressToBytes(ZERO_ADDRESS), - amountToDeposit, - amountReceived, - destinationChainId, - 0, - quoteTimestamp, - BigNumber.from("0xFFFFFFFF"), - 0, - addressToBytes(newDepositor), // Depositor is overridden. - addressToBytes(recipient.address), - addressToBytes(ZERO_ADDRESS), - "0x" - ); - }); - it("Depositing ETH correctly wraps into WETH", async function () { - const revertReason = "MsgValueDoesNotMatchInputAmount"; - - // Fails if msg.value > 0 but doesn't match amount to deposit. - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: weth.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }), - { value: 1 } - ) - ).to.be.revertedWith(revertReason); - - await expect(() => - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - recipient: recipient.address, - originToken: weth.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }), - { value: amountToDeposit } - ) - ).to.changeEtherBalances([depositor, weth], [amountToDeposit.mul(toBN("-1")), amountToDeposit]); // ETH should transfer from depositor to WETH contract. - - // WETH balance for user should be same as start, but WETH balancein pool should increase. - expect(await weth.balanceOf(depositor.address)).to.equal(amountToSeedWallets); - expect(await weth.balanceOf(spokePool.address)).to.equal(amountToDeposit); - }); - - it("Depositing ETH with msg.value = 0 pulls WETH from depositor", async function () { - await expect(() => - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: weth.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }), - { value: 0 } - ) - ).to.changeTokenBalances(weth, [depositor, spokePool], [amountToDeposit.mul(toBN("-1")), amountToDeposit]); - }); - - it("SpokePool is not approved to spend originToken", async function () { - const insufficientAllowance = "ERC20: insufficient allowance"; - - await erc20.connect(depositor).approve(spokePool.address, 0); - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }) - ) - ).to.be.reverted; - - await erc20.connect(depositor).approve(spokePool.address, amountToDeposit); - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp, - }) - ) - ).to.emit(spokePool, "FundsDeposited"); - }); - - it("Relayer fee is invalid", async function () { - const revertReason = "InvalidRelayerFee"; - - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct: toWei("1"), // Fee > 50% - quoteTimestamp, - }) - ) - ).to.be.revertedWith(revertReason); - }); - - it("quoteTimestamp is out of range", async function () { - const revertReason = "InvalidQuoteTimestamp"; - const quoteTimeBuffer = await spokePool.depositQuoteTimeBuffer(); - - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp: quoteTimestamp + 1, - }) - ) - ).to.be.revertedWith("underflowed"); - - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp: quoteTimestamp - (quoteTimeBuffer + 1), - }) - ) - ).to.be.revertedWith(revertReason); - - // quoteTimestamp at the exact margins should succeed. - for (const offset of [0, quoteTimeBuffer]) { - await erc20.connect(depositor).approve(spokePool.address, amountToDeposit); - await expect( - spokePool.connect(depositor).depositDeprecated_5947912356( - ...getDepositParams({ - originToken: erc20.address, - amount, - destinationChainId, - relayerFeePct, - quoteTimestamp: quoteTimestamp - offset, - }) - ) - ).to.emit(spokePool, "FundsDeposited"); - } - }); - it("should call legacy deposit through overloaded interface", async function () { - // Define the deprecated interface - const DeprecatedSpokePoolInterface = new ethers.utils.Interface([ - "function deposit(address recipient, address originToken, uint256 amount, uint256 destinationChainId, int64 relayerFeePct, uint32 quoteTimestamp, bytes memory message, uint256 maxCount) external payable", - ]); - - // Create a new instance of the SpokePool with the deprecated interface - const deprecatedSpokePool = new ethers.Contract(spokePool.address, DeprecatedSpokePoolInterface, depositor); - - // Call the deprecated deposit method - await expect( - await deprecatedSpokePool.deposit( - depositor.address, // recipient - erc20.address, // originToken - amountToDeposit, // amount - destinationChainId, // destinationChainId - 0, // relayerFeePct - quoteTimestamp, // quoteTimestamp - "0x", // message - 0 // maxCount - ) - ).to.emit(spokePool, "FundsDeposited"); - - // Test depositing native ETH directly - await expect( - deprecatedSpokePool.deposit( - depositor.address, // recipient - weth.address, // originToken - still WETH address for native deposits - amountToDeposit, // amount - destinationChainId, // destinationChainId - 0, // relayerFeePct - quoteTimestamp, // quoteTimestamp - "0x", // message - 0, // maxCount - { value: amountToDeposit } // Send ETH - ) - ).to.emit(spokePool, "FundsDeposited"); - }); - describe("deposit V3", function () { let relayData: V3RelayData, depositArgs: any[]; function getDepositArgsFromRelayData( @@ -791,6 +523,16 @@ describe("SpokePool Depositor Logic", async function () { ]); await expect(spokePool.connect(depositor).callback(functionCalldata)).to.be.reverted; }); + it("output token cannot be zero address", async function () { + await expect( + spokePool.connect(depositor).deposit( + ...getDepositArgsFromRelayData({ + ...relayData, + outputToken: ZERO_ADDRESS, + }) + ) + ).to.be.revertedWith("InvalidOutputToken"); + }); 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"; From abc315195b86f67710b3fb0a6d751286ae135ed0 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Mon, 23 Jun 2025 21:59:01 -0400 Subject: [PATCH 2/3] [N-11] Unused Errors Due To Deprecated Logic (#1047) After the removal of the _deposit function from the SpokePool contract, the InvalidRelayerFeePct and MaxTransferSizeExceeded errors are no longer in use. Consider removing the unused errors. --- contracts/interfaces/SpokePoolInterface.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/interfaces/SpokePoolInterface.sol b/contracts/interfaces/SpokePoolInterface.sol index 207708b31..90b58f72e 100644 --- a/contracts/interfaces/SpokePoolInterface.sol +++ b/contracts/interfaces/SpokePoolInterface.sol @@ -56,8 +56,6 @@ interface SpokePoolInterface { error NotEOA(); error InvalidDepositorSignature(); - error InvalidRelayerFeePct(); - error MaxTransferSizeExceeded(); error InvalidCrossDomainAdmin(); error InvalidWithdrawalRecipient(); error DepositsArePaused(); From 0ca65c8250058110a345e08667e09c7b10641133 Mon Sep 17 00:00:00 2001 From: Matt Rice Date: Mon, 23 Jun 2025 22:38:47 -0400 Subject: [PATCH 3/3] [L-13] Function Selectors On Deprecated Functions Are Not Locked (#1048) * [L-13] Function Selectors On Deprecated Functions Are Not Locked Pull request 1031 removes already announced deprecated functionalities, such as the depositDeprecated_5947912356 and depositFor functions, alongside their _deposit internal function. However, as these entry points are removed, future versions of the codebase might introduce new functions that could have the same function selector as the removed ones. In such case, a protocol that might have used the deprecated functionalities could now call to the new ones with unexpected outcomes. Similarly, if a fallback function is used in the future, depending on its implementation, it might take the calldata of protocols using the old deprecated functionalities with similar results. In order to prevent the reuse of the deprecated function selectors, consider keeping the public functions' declaration, without their original definition, and reverting the calls to them. * Add note about selector collision with original deposit function --- contracts/SpokePool.sol | 43 +++++++++++++++++++ contracts/interfaces/SpokePoolInterface.sol | 37 ++++++++++++++++ contracts/interfaces/V3SpokePoolInterface.sol | 1 + 3 files changed, 81 insertions(+) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 71a96aa8e..37bb414de 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -342,6 +342,49 @@ abstract contract SpokePool is emit EmergencyDeletedRootBundle(rootBundleId); } + /************************************** + * REMOVED DEPOSITOR FUNCTIONS * + **************************************/ + + /** + * @dev REMOVED: This function has been removed and is now disallowed to prevent selector reuse. + * @notice This function was removed from the protocol. Calling it will revert. + * Function selector is preserved to prevent accidental reuse in future versions. + * @notice This function shares the same selector as the original "deposit" function that was removed. + * The collision was intentionally created to allow reusing the "deposit" name for a different function signature. + */ + function depositDeprecated_5947912356( + address, + address, + uint256, + uint256, + int64, + uint32, + bytes memory, + uint256 + ) public payable { + revert RemovedFunction(); + } + + /** + * @dev REMOVED: This function has been removed and is now disallowed to prevent selector reuse. + * @notice This function was removed from the protocol. Calling it will revert. + * Function selector is preserved to prevent accidental reuse in future versions. + */ + function depositFor( + address, + address, + address, + uint256, + uint256, + int64, + uint32, + bytes memory, + uint256 + ) public payable { + revert RemovedFunction(); + } + /******************************************** * DEPOSITOR FUNCTIONS * ********************************************/ diff --git a/contracts/interfaces/SpokePoolInterface.sol b/contracts/interfaces/SpokePoolInterface.sol index 90b58f72e..2b4bf85ba 100644 --- a/contracts/interfaces/SpokePoolInterface.sol +++ b/contracts/interfaces/SpokePoolInterface.sol @@ -46,6 +46,43 @@ interface SpokePoolInterface { function emergencyDeleteRootBundle(uint256 rootBundleId) external; + // REMOVED FUNCTIONS: These functions have been removed and are now disallowed. + // Function selectors are preserved to prevent accidental reuse in future versions. + // All calls to these functions will revert. + + /** + * @dev REMOVED: This function has been removed and is now disallowed. + * @notice Calling this function will revert. Use deposit() or depositV3() instead. + * @notice This function shares the same selector as the original "deposit" function that was removed. + * The collision was intentionally created to allow reusing the "deposit" name for a different function signature. + */ + function depositDeprecated_5947912356( + address recipient, + address originToken, + uint256 amount, + uint256 destinationChainId, + int64 relayerFeePct, + uint32 quoteTimestamp, + bytes memory message, + uint256 maxCount + ) external payable; + + /** + * @dev REMOVED: This function has been removed and is now disallowed. + * @notice Calling this function will revert. Use deposit() or depositV3() instead. + */ + function depositFor( + address depositor, + address recipient, + address originToken, + uint256 amount, + uint256 destinationChainId, + int64 relayerFeePct, + uint32 quoteTimestamp, + bytes memory message, + uint256 maxCount + ) external payable; + function executeRelayerRefundLeaf( uint32 rootBundleId, SpokePoolInterface.RelayerRefundLeaf memory relayerRefundLeaf, diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol index 249ceabb2..a22fab443 100644 --- a/contracts/interfaces/V3SpokePoolInterface.sol +++ b/contracts/interfaces/V3SpokePoolInterface.sol @@ -328,6 +328,7 @@ interface V3SpokePoolInterface { error InvalidFillDeadline(); error InvalidExclusiveRelayer(); error InvalidOutputToken(); + error RemovedFunction(); error MsgValueDoesNotMatchInputAmount(); error NotExclusiveRelayer(); error NoSlowFillsInExclusivityWindow();