feat: disallow deposits with output token set to 0x0 (#1031)

* feat: disallow deposits with output token set to 0x0

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

* [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.

* [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

* update for no address(0) changes

Signed-off-by: Taylor Webb <tbwebb22@gmail.com>

---------

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Taylor Webb <tbwebb22@gmail.com>
Co-authored-by: Taylor Webb <tbwebb22@gmail.com>

Author: mrice32

contracts/SpokePool.sol

@@ -395,87 +395,46 @@ abstract contract SpokePool is
     }
 
     /**************************************
-     *    LEGACY DEPOSITOR FUNCTIONS      *
+     *      REMOVED 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.
+     * @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 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
-        );
+        address,
+        address,
+        uint256,
+        uint256,
+        int64,
+        uint32,
+        bytes memory,
+        uint256
+    ) public payable {
+        revert RemovedFunction();
     }
 
     /**
-     * @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.
+     * @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 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);
+        address,
+        address,
+        address,
+        uint256,
+        uint256,
+        int64,
+        uint32,
+        bytes memory,
+        uint256
+    ) public payable {
+        revert RemovedFunction();
     }
 
     /********************************************
@@ -1342,6 +1301,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
@@ -1417,71 +1379,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,

contracts/interfaces/SpokePoolInterface.sol

@@ -46,6 +46,16 @@ 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,
@@ -57,6 +67,10 @@ interface SpokePoolInterface {
         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,
@@ -79,8 +93,6 @@ interface SpokePoolInterface {
 
     error NotEOA();
     error InvalidDepositorSignature();
-    error InvalidRelayerFeePct();
-    error MaxTransferSizeExceeded();
     error InvalidCrossDomainAdmin();
     error InvalidWithdrawalRecipient();
     error DepositsArePaused();

contracts/interfaces/V3SpokePoolInterface.sol

@@ -327,6 +327,8 @@ interface V3SpokePoolInterface {
     error InvalidQuoteTimestamp();
     error InvalidFillDeadline();
     error InvalidExclusiveRelayer();
+    error InvalidOutputToken();
+    error RemovedFunction();
     error MsgValueDoesNotMatchInputAmount();
     error NotExclusiveRelayer();
     error NoSlowFillsInExclusivityWindow();