improve: enforce OFT fee payment by caller (#1082)

* enforce payment of Oft fee by caller

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* revert version bump

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* fix forge tests

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* fix hardhat tests

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* remove unused imports

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* only wrap native token if the l2 token is not OFT

Signed-off-by: Ihor Farion <ihor@umaproject.org>

---------

Signed-off-by: Ihor Farion <ihor@umaproject.org>

Author: grasphoper

contracts/Arbitrum_SpokePool.sol

@@ -91,7 +91,7 @@ contract Arbitrum_SpokePool is SpokePool, CircleCCTPAdapter {
         if (_isCCTPEnabled() && l2TokenAddress == address(usdcToken)) {
             _transferUsdc(withdrawalRecipient, amountToReturn);
         } else if (oftMessenger != address(0)) {
-            _transferViaOFT(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
+            _fundedTransferViaOft(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
         } else {
             // Check that the Ethereum counterpart of the L2 token is stored on this contract.
             address ethereumTokenToBridge = whitelistedTokens[l2TokenAddress];

contracts/Polygon_SpokePool.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 import "./SpokePool.sol";
 import "./PolygonTokenBridger.sol";
 import "./external/interfaces/WETH9Interface.sol";
+import { IOFT } from "./interfaces/IOFT.sol";
 import "./interfaces/SpokePoolInterface.sol";
 import "./libraries/CircleCCTPAdapter.sol";
 
@@ -237,9 +238,11 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool, CircleCCTPAdapter
         emit SetPolygonTokenBridger(address(_polygonTokenBridger));
     }
 
-    function _preExecuteLeafHook(address) internal override {
-        // Wraps MATIC --> WMATIC before distributing tokens from this contract.
-        _wrap();
+    function _preExecuteLeafHook(address l2TokenAddress) internal override {
+        // Wraps MATIC --> WMATIC before distributing tokens from this contract. Only wrap if the token is not an OFT token
+        if (_getOftMessenger(l2TokenAddress) == address(0)) {
+            _wrap();
+        }
     }
 
     function _bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) internal override {
@@ -253,7 +256,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool, CircleCCTPAdapter
         if (_isCCTPEnabled() && l2TokenAddress == address(usdcToken)) {
             _transferUsdc(withdrawalRecipient, amountToReturn);
         } else if (oftMessenger != address(0)) {
-            _transferViaOFT(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
+            _fundedTransferViaOft(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
         } else {
             PolygonIERC20Upgradeable(l2TokenAddress).safeIncreaseAllowance(
                 address(polygonTokenBridger),

contracts/SpokePool.sol

@@ -12,7 +12,7 @@ import "./upgradeable/MultiCallerUpgradeable.sol";
 import "./upgradeable/EIP712CrossChainUpgradeable.sol";
 import "./upgradeable/AddressLibUpgradeable.sol";
 import "./libraries/AddressConverters.sol";
-import { IOFT } from "./interfaces/IOFT.sol";
+import { IOFT, SendParam, MessagingFee } from "./interfaces/IOFT.sol";
 import { OFTTransportAdapter } from "./libraries/OFTTransportAdapter.sol";
 
 import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
@@ -203,6 +203,8 @@ abstract contract SpokePool is
     event SetOFTMessenger(address indexed token, address indexed messenger);
 
     error OFTTokenMismatch();
+    /// @notice Thrown when the native fee sent by the caller is insufficient to cover the OFT transfer.
+    error OFTFeeUnderpaid();
 
     /**
      * @notice Construct the SpokePool. Normally, logic contracts used in upgradeable proxies shouldn't
@@ -1750,6 +1752,30 @@ abstract contract SpokePool is
         return oftMessengers[_token];
     }
 
+    /**
+     * @notice Perform an OFT transfer where the caller supplies the native fee via msg.value.
+     * @dev Supports overpayment: any excess native token is refunded to msg.sender before executing the transfer.
+     *      This function does not re-quote and uses the provided `fee` with `_sendOftTransfer`.
+     *      Must be invoked from a payable context.
+     * @param _token ERC-20 token to transfer.
+     * @param _messenger OFT messenger contract on the current chain for `_token`.
+     * @param _to Destination address on the remote chain.
+     * @param _amount Amount of tokens to transfer.
+     */
+    function _fundedTransferViaOft(IERC20 _token, IOFT _messenger, address _to, uint256 _amount) internal {
+        (SendParam memory sendParam, MessagingFee memory fee) = _buildOftTransfer(_messenger, _to, _amount);
+
+        if (fee.nativeFee > msg.value) {
+            revert OFTFeeUnderpaid();
+        }
+        // Refund any overpayment to the caller using a safe native transfer.
+        uint256 refund = msg.value - fee.nativeFee;
+        if (refund > 0) {
+            AddressLibUpgradeable.sendValue(payable(msg.sender), refund);
+        }
+        _sendOftTransfer(_token, _messenger, sendParam, fee);
+    }
+
     // 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.

contracts/Universal_SpokePool.sol

@@ -185,7 +185,7 @@ contract Universal_SpokePool is OwnableUpgradeable, SpokePool, CircleCCTPAdapter
         if (_isCCTPEnabled() && l2TokenAddress == address(usdcToken)) {
             _transferUsdc(withdrawalRecipient, amountToReturn);
         } else if (oftMessenger != address(0)) {
-            _transferViaOFT(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
+            _fundedTransferViaOft(IERC20(l2TokenAddress), IOFT(oftMessenger), withdrawalRecipient, amountToReturn);
         } else {
             revert NotImplemented();
         }

contracts/libraries/OFTTransportAdapter.sol

@@ -67,6 +67,25 @@ contract OFTTransportAdapter {
      * @param _amount amount to send.
      */
     function _transferViaOFT(IERC20 _token, IOFT _messenger, address _to, uint256 _amount) internal {
+        (SendParam memory sendParam, MessagingFee memory fee) = _buildOftTransfer(_messenger, _to, _amount);
+        _sendOftTransfer(_token, _messenger, sendParam, fee);
+    }
+
+    /**
+     * @notice Build OFT send params and quote the native fee.
+     * @dev Sets `minAmountLD == amountLD` to disallow silent deductions (e.g. dust removal) by OFT.
+     *      The fee is quoted for payment in native token.
+     * @param _messenger OFT messenger contract on the current chain for the token being sent.
+     * @param _to Destination address on the remote chain.
+     * @param _amount Amount of tokens to transfer.
+     * @return sendParam The encoded OFT send parameters.
+     * @return fee The quoted MessagingFee required for the transfer.
+     */
+    function _buildOftTransfer(
+        IOFT _messenger,
+        address _to,
+        uint256 _amount
+    ) internal view returns (SendParam memory, MessagingFee memory) {
         bytes32 to = _to.toBytes32();
 
         SendParam memory sendParam = SendParam(
@@ -90,13 +109,32 @@ contract OFTTransportAdapter {
 
         // `false` in the 2nd param here refers to `bool _payInLzToken`. We will pay in native token, so set to `false`
         MessagingFee memory fee = _messenger.quoteSend(sendParam, false);
+
+        return (sendParam, fee);
+    }
+
+    /**
+     * @notice Execute an OFT transfer using pre-built params and fee.
+     * @dev Verifies fee bounds and equality of sent/received amounts. Pays native fee from this contract.
+     * @param _token ERC-20 token to transfer.
+     * @param _messenger OFT messenger contract on the current chain for `_token`.
+     * @param sendParam Pre-built OFT send parameters.
+     * @param fee Quoted MessagingFee to pay for this transfer.
+     */
+    function _sendOftTransfer(
+        IERC20 _token,
+        IOFT _messenger,
+        SendParam memory sendParam,
+        MessagingFee memory fee
+    ) internal {
         // Create a stack variable to optimize gas usage on subsequent reads
         uint256 nativeFee = fee.nativeFee;
         if (nativeFee > OFT_FEE_CAP) revert OftFeeCapExceeded();
         if (nativeFee > address(this).balance) revert OftInsufficientBalanceForFee();
         if (fee.lzTokenFee != 0) revert OftLzFeeNotZero();
 
         // Approve the exact _amount for `_messenger` to spend. Fee will be paid in native token
+        uint256 _amount = sendParam.amountLD;
         _token.forceApprove(address(_messenger), _amount);
 
         (, OFTReceipt memory oftReceipt) = _messenger.send{ value: nativeFee }(sendParam, fee, address(this));

test/evm/foundry/local/Universal_SpokePool.t.sol

@@ -6,6 +6,7 @@ import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 
 import { Universal_SpokePool, IHelios } from "../../../../contracts/Universal_SpokePool.sol";
+import "../../../../contracts/SpokePool.sol";
 import "../../../../contracts/libraries/CircleCCTPAdapter.sol";
 import "../../../../contracts/test/MockCCTP.sol";
 import { IOFT, SendParam, MessagingFee } from "../../../../contracts/interfaces/IOFT.sol";
@@ -26,11 +27,7 @@ contract MockHelios is IHelios {
         headTimestamp = _timestamp;
     }
 
-    function getStorageSlot(
-        uint256,
-        address,
-        bytes32 _key
-    ) external view returns (bytes32) {
+    function getStorageSlot(uint256, address, bytes32 _key) external view returns (bytes32) {
         return storageSlots[_key];
     }
 }
@@ -62,7 +59,7 @@ contract MockUniversalSpokePool is Universal_SpokePool {
         )
     {}
 
-    function test_bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) external {
+    function test_bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) external payable {
         _bridgeTokensToHubPool(amountToReturn, l2TokenAddress);
     }
 }
@@ -365,13 +362,10 @@ contract UniversalSpokePoolTest is Test {
         spokePool.executeMessage(nonce, value, 100);
         nonce++; // Increment nonce for the next message
 
-        // Fund the spokePool with enough native currency to attempt the transaction but less than the high fee
-        // The check for OFT_FEE_CAP happens before the balance check.
-        deal(address(spokePool), spokePool.OFT_FEE_CAP());
-
-        // Expect the OftFeeCapExceeded error from OFTTransportAdapter
+        // Expect the OftFeeCapExceeded error from OFTTransportAdapter.
+        // Provide msg.value to cover the quoted fee so the cap guard is triggered.
         vm.expectRevert(OFTTransportAdapter.OftFeeCapExceeded.selector);
-        spokePool.test_bridgeTokensToHubPool(usdcMintAmount, address(usdt));
+        spokePool.test_bridgeTokensToHubPool{ value: highNativeFee }(usdcMintAmount, address(usdt));
     }
 
     function testBridgeTokensToHubPool_oft() public {
@@ -423,12 +417,9 @@ contract UniversalSpokePoolTest is Test {
         spokePool.executeMessage(nonce, value, 100);
         nonce++;
 
-        // Ensure spokePool has less balance than nativeFee
-        deal(address(spokePool), nativeFee - 1);
-
-        // Expect revert due to insufficient balance for fee
-        vm.expectRevert(OFTTransportAdapter.OftInsufficientBalanceForFee.selector);
-        spokePool.test_bridgeTokensToHubPool(usdcMintAmount, address(usdt));
+        // Expect revert due to caller underpaying the required OFT native fee
+        vm.expectRevert(SpokePool.OFTFeeUnderpaid.selector);
+        spokePool.test_bridgeTokensToHubPool{ value: nativeFee - 1 }(usdcMintAmount, address(usdt));
     }
 
     function testBridgeTokensToHubPool_oft_incorrectAmountReceived() public {

test/evm/hardhat/chain-specific-spokepools/Arbitrum_SpokePool.ts

@@ -235,16 +235,8 @@ describe("Arbitrum Spoke Pool", function () {
       ] as MessagingFeeStructOutput;
       l2OftMessenger.quoteSend.returns(msgFeeStruct);
 
-      // Fund the spoke pool with enough ETH to cover the fee as `executeRelayerRefundLeaf` is not payable.
-      await hre.network.provider.send("hardhat_setBalance", [
-        arbitrumSpokePool.address,
-        // @dev Notice, this value is very sensitive to change. It wants a hex string WITHOUT leading zeroes after 0x, whereas
-        // BigNumber's .toHexString() can easily return a leading zero (probably because just converting from bytes)
-        toWei("2").toHexString(),
-      ]);
-
       await expect(
-        arbitrumSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]))
+        arbitrumSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]), { value: nativeFee })
       ).to.be.revertedWith("OftLzFeeNotZero");
     });
 
@@ -256,31 +248,22 @@ describe("Arbitrum Spoke Pool", function () {
       ] as MessagingFeeStructOutput;
       l2OftMessenger.quoteSend.returns(msgFeeStruct);
 
-      // Fund the spoke pool with enough ETH to cover the fee as `executeRelayerRefundLeaf` is not payable.
-      await hre.network.provider.send("hardhat_setBalance", [
-        arbitrumSpokePool.address,
-        // @dev Notice, this value is very sensitive to change. It wants a hex string WITHOUT leading zeroes after 0x, whereas
-        // BigNumber's .toHexString() can easily return a leading zero (probably because just converting from bytes)
-        highNativeFee.toHexString(),
-      ]);
-
       await expect(
-        arbitrumSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]))
+        arbitrumSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]), { value: highNativeFee })
       ).to.be.revertedWith("OftFeeCapExceeded");
     });
 
-    it("reverts with OftInsufficientBalanceForFee if spoke pool has not enough ETH for fee", async function () {
+    it("reverts with OFTFeeUnderpaid if caller does not supply the required OFT fee", async function () {
       const nativeFee = toWei("0.1");
       const msgFeeStruct: MessagingFeeStructOutput = [
         nativeFee, // nativeFee
         BigNumber.from(0), // lzTokenFee
       ] as MessagingFeeStructOutput;
       l2OftMessenger.quoteSend.returns(msgFeeStruct);
 
-      // Note: the `executeRelayerRefundLeaf` is not payable, so the native fee must be covered by the contract's balance
       await expect(
         arbitrumSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]))
-      ).to.be.revertedWith("OftInsufficientBalanceForFee");
+      ).to.be.revertedWith("OFTFeeUnderpaid");
     });
 
     it("reverts with OftIncorrectAmountReceivedLD if OFT receipt has wrong received amount", async function () {

test/evm/hardhat/chain-specific-spokepools/Polygon_SpokePool.ts

@@ -195,7 +195,7 @@ describe("Polygon Spoke Pool", function () {
     expect(l2OftMessenger.send).to.have.been.calledWith(sendParam, msgFeeStruct, polygonSpokePool.address);
   });
 
-  it("reverts with OftInsufficientBalanceForFee if spoke pool has not enough ETH for OFT fee", async function () {
+  it("reverts with OFTFeeUnderpaid if caller does not supply the required OFT fee", async function () {
     l2OftMessenger.token.returns(l2UsdtContract.address);
     const setOftMessengerData = polygonSpokePool.interface.encodeFunctionData("setOftMessenger", [
       l2UsdtContract.address,
@@ -221,7 +221,7 @@ describe("Polygon Spoke Pool", function () {
 
     await expect(
       polygonSpokePool.executeRelayerRefundLeaf(0, leaves[0], tree.getHexProof(leaves[0]))
-    ).to.be.revertedWith("OftInsufficientBalanceForFee");
+    ).to.be.revertedWith("OFTFeeUnderpaid");
   });
 
   it("Only correct caller can set the cross domain admin", async function () {