Skip to content

Latest commit

 

History

History
227 lines (181 loc) · 11.3 KB

092.md

File metadata and controls

227 lines (181 loc) · 11.3 KB

Uneven Satin Lizard

High

All swapOnFill= true stop limit orders cannot be executed due to wrong StopLimit::performData::target and StopLimit::performData::txData, breaking core contract functionality

Summary

StopLimit::performUpkeep executes swapOnFill= true stop limit orders that are in range by calling the StopLimit::performData::target address with StopLimit::performData::txData function call to perform the swap and create a new bracket order. However, StopLimit::performData::target and StopLimit::performData::txData are wrongly set to address(this) (StopLimit contract) and abi.encodePacked(swapOnFill) respectively in StopLimit.sol#L57-58. This causes swapOnFill= true stop limit orders to fail and revert eventhough they are in range, breaking core contract functionality.

Root Cause

StopLimit.sol#L57-58 is set to the wrong target address and txData function call.

Internal pre-conditions

  1. Max pending orders is not exceeded, i.e. owner calls AutomationMaster::setMaxPendingOrders to set AutomationMaster::maxPendingOrders to a non-zero value
  2. User's order exceeds min order size (simulated in this PoC by owner calling AutomationMaster::setMinOrderSize to set AutomationMaster::minOrderSize to 0 so all orders pass)
  3. User has some tokenIn and has approved StopLimit contract for amountIn to create the swapOnFill= true stop limit order

External pre-conditions

  1. Orders are inRange. Price of tokenIn and tokenOut provided by oracles must be such that exchangeRate <= stopLimitPrice (for direction = true) or exchangeRate >= stopLimitPrice (for direction = false)

Attack Path

  1. User calls StopLimit::createOrder with swapOnFill = true to create a swapOnFill stop limit order
  2. Price of tokenIn and tokenOut provided by oracles changes such that exchangeRate <= stopLimitPrice (for direction = true) or exchangeRate >= stopLimitPrice (for direction = false), i.e. orders are inRange
  3. Keepers calls AutomationMaster::checkUpkeep (which are passed to StopLimit::checkUpkeep) to check if upKeep is needed
    3.1. Since orders are inRange, upKeep is needed (upKeepNeeded = true)
    3.2. performData is set with a MasterUpkeepData struct with target = address(this) (StopLimit contract) and txData = abi.encodePacked(swapOnFill), along with other arguments \
  4. Keepers calls AutomationMaster::performUpkeep with the performData returned by checkUpkeep. This executes a series of function calls as follows:
    4.1. AutomationMaster::performUpkeep calls StopLimit::performUpkeep with performData arguments
    4.2. StopLimit::performUpkeep calls StopLimit::updateApproval to provide ERC20 approval for Bracket contract
    4.3. StopLimit::performUpkeep creates and encodes a SwapParams struct with SwapParams.swapTarget = performData.target (which is address(this), i.e. StopLimit contract) and SwapParams.txData = performData.txData (which is abi.encodePacked(swapOnFill))
    4.4. StopLimit::performUpkeep calls Bracket::fillStopLimitOrder with the SwapParams (along with other arguments)
    4.5. Bracket::fillStopLimitOrder calls Bracket::_initializeOrder with the SwapParams (along with other arguments)
    4.6. Bracket::_initializeOrder calls Bracket::procureTokens to retrieve the ERC20 token from StopLimit contract using the approval in 4.2.
    4.7. Bracket::_initializeOrder calls Bracket::_createOrderWithSwap with the SwapParams (along with other arguments)
    4.8. Bracket::_createOrderWithSwapcalls Bracket::execute with SwapParams.swapTarget (which is StopLimit contract) and SwapParams.txData (which is abi.encodePacked(swapOnFill))
    4.9. Bracket::execute approves SwapParams.swapTarget for SwapParams.swapAmountIn of SwapParams.swapTokenIn
    4.10. Bracket::execute calls SwapParams.swapTarget with SwapParams.txData function call to perform the swap. Since the SwapParams.target is the StopLimit contract, Bracket::execute will call the StopLimit contract with SwapParams.txData.
    4.11. Since the StopLimit contract does not have function selector matching SwapParams.txData (abi.encodePacked(swapOnFill)) nor a fallback function, the call returns success = false.
    4.12. Bracket::execute reverts with TransactionFailed("0x")

Impact


Impact: High. None of the swapOnFill= true stop limit orders that are in range can be executed, breaking core contract functionality.
Likelihood: High. The bug is always present as all swapOnFill= true stop limit orders are executed through StopLimit::performData::target with function call StopLimit::performData::txData.
Severity: High

PoC

The below PoC is written in foundry. To set up the environment, run the following code in the oku-custom-order-types directory (ref).

  1. Install foundry

    curl -L https://foundry.paradigm.xyz | bash

  2. Install the @nomicfoundation/hardhat-foundry plugin

    npm install --save-dev @nomicfoundation/hardhat-foundry

  3. Import the plugin into hardhat.config.cts

    import "@nomicfoundation/hardhat-foundry";

  4. Initialize foundry configuration file (foundry.toml) and install forge-std using the below

    npx hardhat init-foundry

  5. Install libraries and dependencies required for testing

    forge install openzeppelin/openzeppelin-contracts --no-commit


The PoC below tests swapOnFill = true stop limit orders. Place the following PoC into test/StopLimit.t.sol and run the following

forge test --mt testStopLimitSwapOnFillFails --via-ir

// SPDX-License-Identifer: MIT

pragma solidity ^0.8.20;

import {Test} from "forge-std/Test.sol";
import {AutomationMaster, IAutomation} from "contracts/automatedTrigger/AutomationMaster.sol";
import {Bracket, IBracket} from "contracts/automatedTrigger/Bracket.sol";
import {StopLimit, IStopLimit} from "contracts/automatedTrigger/StopLimit.sol";
import {ERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IERC20} from "contracts/interfaces/openzeppelin/IERC20.sol";
import {IPythRelay} from "contracts/oracle/IPythRelay.sol";
import {IPermit2} from "contracts/interfaces/uniswapV3/IPermit2.sol";

contract BracketTest is Test {
    address owner = makeAddr("owner");
    address keeper = makeAddr("chainlink");
    // uniswapV3 permit not used in this PoC, just dummy address
    address permit2 = makeAddr("permit2");
    address user = makeAddr("user");

    AutomationMaster MASTER;
    Bracket bracket;
    StopLimit stopLimit;
    MockWETHOracle wethOracle;
    MockUSDxOracle usdxOracle;
    ERC20Mock weth;
    ERC20Mock usdx;

    uint256 constant WETH_INITIAL_BALANCE = 1e18;

    function setUp() public {
        // Deploy contracts
        vm.startPrank(owner);
        MASTER = new AutomationMaster();
        bracket = new Bracket(MASTER, IPermit2(permit2));
        stopLimit = new StopLimit(MASTER, bracket, IPermit2(permit2));
        vm.stopPrank();

        // Deploy mock tokens
        weth = new ERC20Mock("WETH", "WETH");
        usdx = new ERC20Mock("USDx", "USDx");

        // Deploy and initialize mock oracles
        wethOracle = new MockWETHOracle();
        usdxOracle = new MockUSDxOracle();

        // Register mock oracles
        IERC20[] memory _tokens = new IERC20[](2);
        _tokens[0] = IERC20(address(weth));
        _tokens[1] = IERC20(address(usdx));
        IPythRelay[] memory _oracles = new IPythRelay[](2);
        _oracles[0] = IPythRelay(address(wethOracle));
        _oracles[1] = IPythRelay(address(usdxOracle));
        vm.prank(owner);
        MASTER.registerOracle(_tokens, _oracles);

        // Register subKeepers
        vm.prank(owner);
        MASTER.registerSubKeepers(IStopLimit(stopLimit), IBracket(bracket));

        // Assume WETH price
        wethOracle.setPrice(3000e8); // 3000 WETH/USD

        // Condition 1: Max pending orders is not exceeded
        vm.prank(owner);
        MASTER.setMaxPendingOrders(type(uint16).max);

        // Condition 2: Min order size is obeyed
        // simulated by setting minOrderSize = 0 so all orders pass
        vm.prank(owner);
        MASTER.setMinOrderSize(0);

        // Condition 3: User has some tokenIn (WETH) and has approved bracket contract for amountIn
        weth.mint(user, WETH_INITIAL_BALANCE);
        vm.prank(user);
        weth.approve(address(stopLimit), WETH_INITIAL_BALANCE);
    }

    function testStopLimitSwapOnFillFails() public {
        // Step 1: User makes stop limit order
        uint256 stopLimitPrice = 3000e8; // 3000 WETH/USD
        uint256 takeProfit = 4000e8; // 4000 WETH/USD
        uint256 stopPrice = 2000e8; // 2000 WETH/USD
        uint256 amountIn = WETH_INITIAL_BALANCE;
        IERC20 tokenIn = IERC20(address(weth));
        IERC20 tokenOut = IERC20(address(usdx));
        address recipient = user;
        uint16 feeBips = 0;
        uint16 takeProfitSlippage = 100; // 100 BIPS
        uint16 stopSlippage = 100; // 100 BIPS
        uint16 swapSlippage = 100; // 100 BIPS
        bool swapOnFill = true;
        bool permit = false;
        bytes memory permitPayload = "";

        vm.prank(user);
        stopLimit.createOrder(stopLimitPrice, takeProfit, stopPrice, amountIn, tokenIn, tokenOut, recipient, feeBips, takeProfitSlippage, stopSlippage, swapSlippage, swapOnFill, permit, permitPayload);

        // Step 2: Order is InRange
        // WETH price = stopLimitPrice
        wethOracle.setPrice(3000e8); // 3000 WETH/USD

        // Step 3: Keeper calls checkUpkeep
        vm.prank(keeper);
        (bool upkeepNeeded, bytes memory performData) = MASTER.checkUpkeep("");
        IAutomation.MasterUpkeepData memory data = abi.decode(performData, (IAutomation.MasterUpkeepData));
        assertTrue(upkeepNeeded);
        assertEq(data.target, address(stopLimit));
        assertEq(data.txData, abi.encodePacked(true));

        // Step 4: UpKeep needed, Keeper calls performUpkeep
        vm.prank(keeper);
        vm.expectRevert();
        MASTER.performUpkeep(performData);
    }
}

contract MockWETHOracle {
    uint256 WETHUSDprice;
    
    function setPrice(uint256 price) external {
        WETHUSDprice = price;
    }

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

contract MockUSDxOracle {
    uint256 USDXUSDprice = 1e8;

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

contract ERC20Mock is ERC20 {
    constructor(
        string memory name,
        string memory symbol
    )
        payable
        ERC20(name, symbol)
    {}

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }
}

Mitigation

In StopLimit.sol#L57-58, set target to a DEx and set txData to function selector and arguments of a DEx swap.