fix: prevent setting fee recipient to self (#7154)

yorhodes · web-flow · commit 867997deea0e · 2025-10-13T16:55:26.000-04:00
diff --git a/solidity/contracts/token/extensions/HypERC4626Collateral.sol b/solidity/contracts/token/extensions/HypERC4626Collateral.sol
@@ -82,17 +82,16 @@ contract HypERC4626Collateral is TokenRouter {
     ) public payable override returns (bytes32 messageId) {
         // 1. Calculate the fee amounts, charge the sender and distribute to feeRecipient if necessary
         // Don't use HypERC4626Collateral's implementation of _transferTo since it does a redemption.
-        uint256 feeRecipientFee = _feeRecipientAmount(
+        (address _feeRecipient, uint256 feeAmount) = _feeRecipientAndAmount(
             _destination,
             _recipient,
             _amount
         );
-        _transferFromSender(_amount + feeRecipientFee);
-        if (feeRecipientFee > 0) {
-            wrappedToken._transferTo(feeRecipient(), feeRecipientFee);
+        _transferFromSender(_amount + feeAmount);
+        if (feeAmount > 0) {
+            wrappedToken._transferTo(_feeRecipient, feeAmount);
         }
 
-        // 2. Prepare the token message with the recipient, amount, and any additional metadata in overrides
         // Deposit the amount into the vault and get the shares for the TokenMessage amount
         uint256 _shares = _depositIntoVault(_amount);
 
diff --git a/solidity/contracts/token/libs/TokenRouter.sol b/solidity/contracts/token/libs/TokenRouter.sol
@@ -6,6 +6,7 @@ import {TypeCasts} from "../../libs/TypeCasts.sol";
 import {GasRouter} from "../../client/GasRouter.sol";
 import {TokenMessage} from "./TokenMessage.sol";
 import {Quote, ITokenBridge, ITokenFee} from "../../interfaces/ITokenBridge.sol";
+import {Quotes} from "./Quotes.sol";
 import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol";
 
 /**
@@ -24,6 +25,7 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
     using TypeCasts for address;
     using TokenMessage for bytes;
     using StorageSlot for bytes32;
+    using Quotes for Quote[];
 
     /**
      * @dev Emitted on `transferRemote` when a transfer message is dispatched.
@@ -80,7 +82,7 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
     /**
      * @inheritdoc ITokenFee
      * @notice Implements the standardized fee quoting interface for token transfers based on
-     * overridable internal functions of _quoteGasPayment, _feeRecipientAmount, and _externalFeeAmount.
+     * overridable internal functions of _quoteGasPayment, _feeRecipientAndAmount, and _externalFeeAmount.
      * @param _destination The identifier of the destination chain.
      * @param _recipient The address of the recipient on the destination chain.
      * @param _amount The amount or identifier of tokens to be sent to the remote recipient
@@ -103,11 +105,12 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
             token: address(0),
             amount: _quoteGasPayment(_destination, _recipient, _amount)
         });
-        quotes[1] = Quote({
-            token: token(),
-            amount: _amount +
-                _feeRecipientAmount(_destination, _recipient, _amount)
-        });
+        (, uint256 feeAmount) = _feeRecipientAndAmount(
+            _destination,
+            _recipient,
+            _amount
+        );
+        quotes[1] = Quote({token: token(), amount: _amount + feeAmount});
         quotes[2] = Quote({
             token: token(),
             amount: _externalFeeAmount(_destination, _recipient, _amount)
@@ -175,18 +178,18 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
         uint256 _amount,
         uint256 _msgValue
     ) internal returns (uint256 externalFee, uint256 remainingNativeValue) {
-        uint256 feeRecipientFee = _feeRecipientAmount(
+        (address _feeRecipient, uint256 feeAmount) = _feeRecipientAndAmount(
             _destination,
             _recipient,
             _amount
         );
         externalFee = _externalFeeAmount(_destination, _recipient, _amount);
-        uint256 charge = _amount + feeRecipientFee + externalFee;
+        uint256 charge = _amount + feeAmount + externalFee;
         _transferFromSender(charge);
-        if (feeRecipientFee > 0) {
+        if (feeAmount > 0) {
             // transfer atomically so we don't need to keep track of collateral
             // and fee balances separately
-            _transferTo(feeRecipient(), feeRecipientFee);
+            _transferTo(_feeRecipient, feeAmount);
         }
         remainingNativeValue = token() != address(0)
             ? _msgValue
@@ -221,11 +224,12 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
     /**
      * @notice Sets the fee recipient for the router.
      * @dev Allows for address(0) to be set, which disables fees.
-     * @param _feeRecipient The address of the fee recipient.
+     * @param recipient The address that receives fees.
      */
-    function setFeeRecipient(address _feeRecipient) public onlyOwner {
-        FEE_RECIPIENT_SLOT.getAddressSlot().value = _feeRecipient;
-        emit FeeRecipientSet(_feeRecipient);
+    function setFeeRecipient(address recipient) public onlyOwner {
+        require(recipient != address(this), "Fee recipient cannot be self");
+        FEE_RECIPIENT_SLOT.getAddressSlot().value = recipient;
+        emit FeeRecipientSet(recipient);
     }
 
     /**
@@ -264,32 +268,34 @@ abstract contract TokenRouter is GasRouter, ITokenBridge {
      * @param _destination The identifier of the destination chain.
      * @param _recipient The address of the recipient on the destination chain.
      * @param _amount The amount or identifier of tokens to be sent to the remote recipient
+     * @return _feeRecipient The address of the fee recipient.
      * @return feeAmount The fee recipient amount.
      * @dev This function is is not intended to be overridden as storage and logic is contained in TokenRouter.
      */
-    function _feeRecipientAmount(
+    function _feeRecipientAndAmount(
         uint32 _destination,
         bytes32 _recipient,
         uint256 _amount
-    ) internal view returns (uint256 feeAmount) {
-        if (feeRecipient() == address(0)) {
-            return 0;
+    ) internal view returns (address _feeRecipient, uint256 feeAmount) {
+        _feeRecipient = feeRecipient();
+        if (_feeRecipient == address(0)) {
+            return (_feeRecipient, 0);
         }
 
-        Quote[] memory quotes = ITokenFee(feeRecipient()).quoteTransferRemote(
+        Quote[] memory quotes = ITokenFee(_feeRecipient).quoteTransferRemote(
             _destination,
             _recipient,
             _amount
         );
         if (quotes.length == 0) {
-            return 0;
+            return (_feeRecipient, 0);
         }
 
         require(
             quotes.length == 1 && quotes[0].token == token(),
             "FungibleTokenRouter: fee must match token"
         );
-        return quotes[0].amount;
+        feeAmount = quotes[0].amount;
     }
 
     /**
diff --git a/solidity/test/token/HypnativeMovable.t.sol b/solidity/test/token/HypnativeMovable.t.sol
@@ -12,7 +12,23 @@ import {LinearFee} from "contracts/token/fees/LinearFee.sol";
 import "forge-std/Test.sol";
 
 contract MockITokenBridgeEth is ITokenBridge {
-    constructor() {}
+    uint256 public quoteLength;
+    address public quoteToken;
+    uint256 public quoteAmount;
+
+    constructor() {
+        quoteLength = 0;
+    }
+
+    function setQuote(
+        uint256 _length,
+        address _token,
+        uint256 _amount
+    ) external {
+        quoteLength = _length;
+        quoteToken = _token;
+        quoteAmount = _amount;
+    }
 
     function transferRemote(
         uint32 destinationDomain,
@@ -27,7 +43,15 @@ contract MockITokenBridgeEth is ITokenBridge {
         bytes32 recipient,
         uint256 amountOut
     ) external view override returns (Quote[] memory) {
-        return new Quote[](0);
+        Quote[] memory quotes = new Quote[](quoteLength);
+        if (quoteLength == 1) {
+            quotes[0] = Quote({token: quoteToken, amount: quoteAmount});
+        } else if (quoteLength > 1) {
+            // Return multiple quotes for testing
+            quotes[0] = Quote({token: quoteToken, amount: quoteAmount});
+            quotes[1] = Quote({token: address(0), amount: 100});
+        }
+        return quotes;
     }
 }
 
@@ -113,4 +137,92 @@ contract HypNativeMovableTest is Test {
         assertEq(address(router).balance, balance - amount);
         assertEq(address(vtb).balance, amount);
     }
+
+    function test_setFeeRecipient_cannotSetToSelf() public {
+        vm.expectRevert("Fee recipient cannot be self");
+        router.setFeeRecipient(address(router));
+    }
+
+    function test_setFeeRecipient_canSetToOtherAddress() public {
+        address feeRecipient = address(0x123);
+        router.setFeeRecipient(feeRecipient);
+        assertEq(router.feeRecipient(), feeRecipient);
+    }
+
+    function test_setFeeRecipient_canSetToZeroAddress() public {
+        router.setFeeRecipient(address(0x123));
+        assertEq(router.feeRecipient(), address(0x123));
+
+        router.setFeeRecipient(address(0));
+        assertEq(router.feeRecipient(), address(0));
+    }
+
+    function test_feeRecipient_emptyQuotesReturnsZero() public {
+        MockITokenBridgeEth mockFeeRecipient = new MockITokenBridgeEth();
+        // Set to return empty quotes (length 0)
+        mockFeeRecipient.setQuote(0, address(0), 0);
+
+        router.setFeeRecipient(address(mockFeeRecipient));
+
+        // Should not revert and return 0 fee
+        Quote[] memory quotes = router.quoteTransferRemote(
+            destinationDomain,
+            bytes32(uint256(uint160(alice))),
+            1 ether
+        );
+
+        // quotes[1] is the internal fee (amount + fee)
+        assertEq(quotes[1].amount, 1 ether); // no fee added
+    }
+
+    function test_feeRecipient_multipleQuotesReverts() public {
+        MockITokenBridgeEth mockFeeRecipient = new MockITokenBridgeEth();
+        // Set to return 2 quotes (invalid)
+        mockFeeRecipient.setQuote(2, address(0), 0.1 ether);
+
+        router.setFeeRecipient(address(mockFeeRecipient));
+
+        // Should revert with the fee mismatch error
+        vm.expectRevert("FungibleTokenRouter: fee must match token");
+        router.quoteTransferRemote(
+            destinationDomain,
+            bytes32(uint256(uint160(alice))),
+            1 ether
+        );
+    }
+
+    function test_feeRecipient_wrongTokenReverts() public {
+        MockITokenBridgeEth mockFeeRecipient = new MockITokenBridgeEth();
+        // Set to return 1 quote but with wrong token (not address(0) which is the native token)
+        address wrongToken = address(0x456);
+        mockFeeRecipient.setQuote(1, wrongToken, 0.1 ether);
+
+        router.setFeeRecipient(address(mockFeeRecipient));
+
+        // Should revert with the fee mismatch error
+        vm.expectRevert("FungibleTokenRouter: fee must match token");
+        router.quoteTransferRemote(
+            destinationDomain,
+            bytes32(uint256(uint160(alice))),
+            1 ether
+        );
+    }
+
+    function test_feeRecipient_correctTokenSucceeds() public {
+        MockITokenBridgeEth mockFeeRecipient = new MockITokenBridgeEth();
+        // Set to return 1 quote with correct token (address(0) for native)
+        mockFeeRecipient.setQuote(1, address(0), 0.1 ether);
+
+        router.setFeeRecipient(address(mockFeeRecipient));
+
+        // Should succeed and return correct fee
+        Quote[] memory quotes = router.quoteTransferRemote(
+            destinationDomain,
+            bytes32(uint256(uint160(alice))),
+            1 ether
+        );
+
+        // quotes[1] is the internal fee (amount + fee)
+        assertEq(quotes[1].amount, 1.1 ether); // 1 ether + 0.1 ether fee
+    }
 }
