Skip to content

Latest commit

 

History

History
186 lines (145 loc) · 7.22 KB

M-01.md

File metadata and controls

186 lines (145 loc) · 7.22 KB
Raw

Next queue indices may be wrong if items are not processed in order

Summary

The resolveQueuedTrades function present in the BufferRouter contract keeps track of the next queue indices to be processed (both at the user and global level) and may be wrongly calculated if items in the queue aren't processed in order.

Vulnerability Detail

The BufferRouter contract keeps track of which items are next to be processed in the queue using the storage variables userNextQueueIndexToProcess (to track it at the user level) and nextQueueIdToProcess (to track the global index next to be processed).

These values are updated in the resolveQueuedTrades when the keeper processes the queued trades. The userNextQueueIndexToProcess variable is updated by taking the userQueueIndex param (which is the length of the user queued items at the time it was initiated) and adding 1 to it:

userNextQueueIndexToProcess[queuedTrade.user] =
    queuedTrade.userQueueIndex +
    1;

The nextQueueIdToProcess is also updated in a similar way, it takes the queueId from the last element of the OpenTradeParams[] array parameter and it adds 1 to it:

nextQueueIdToProcess = params[params.length - 1].queueId + 1;

Both of these calculations assume that elements are being processed in order, and will end up with inconsistent values if not.

Impact

This will potentially lead to inconsistent values if queued trades are not processed strictly in order, and eventually leave items unprocessed.

If trade A is opened before trade B, meaning trade A queue id will be lower than trade B queue id, and trade B gets resolved first, then nextQueueIdToProcess will be trade B queue id plus 1, while trade A queue id (which is strictly lower) is still unprocessed.

If keepers use the nextQueueIdToProcess value to keep track of upcoming trades to be processed, then this can lead to some trades being missed and ignored.

Code Snippet

The following test reproduces the issue. Here, Alice opens a trade first with queueId == 0, then Bob opens another trade with queueId == 1. The keeper resolves Bob's trade, which ends up leaving nextQueueIdToProcess with a value of 2, while Alice's trade is still in queue.

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

import "forge-std/Test.sol";
import "../src/core/BufferRouter.sol";
import "../src/core/BufferBinaryOptions.sol";
import "../src/core/BufferBinaryPool.sol";
import "../src/core/OptionsConfig.sol";
import "../src/core/ReferralStorage.sol";
import "../src/interfaces/Interfaces.sol";
import "../src/tokens/USDC.sol";
import "../src/tokens/BadERC20.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";

contract AuditPoolTest is Test {
    bytes32 constant ROUTER_ROLE = keccak256("ROUTER_ROLE");
    bytes32 constant OPTION_ISSUER_ROLE = keccak256("OPTION_ISSUER_ROLE");

    event OpenTrade(address indexed account, uint256 queueId, uint256 optionId);

    address owner;
    uint256 publisherPrivateKey;
    address publisher;
    address feeRecipient;
    address keeper;
    address alice;
    address bob;

    function setUp() public {
        owner = makeAddr("owner");
        publisherPrivateKey = 0x1234;
        publisher = vm.addr(publisherPrivateKey);
        feeRecipient = makeAddr("feeRecipient");
        keeper = makeAddr("keeper");
        alice = makeAddr("alice");
        bob = makeAddr("bob");
    }

    function test_BuffeRouter_resolveQueuedTrades_IncorrectNextQueueIds() public {
        // Setup
        vm.startPrank(owner);

        USDC token = new USDC();
        BufferBinaryPool pool = new BufferBinaryPool(token);
        OptionsConfig config = new OptionsConfig(pool);
        ReferralStorage referral = new ReferralStorage();
        BufferBinaryOptions binary = new BufferBinaryOptions(
            token,
            pool,
            config,
            referral,
            IBufferBinaryOptions.AssetCategory.Crypto,
            "ETH/USDC"
        );
        BufferRouter router = new BufferRouter(publisher);

        config.setSettlementFeeDisbursalContract(feeRecipient);

        router.setContractRegistry(address(binary), true);
        router.setKeeper(keeper, true);

        binary.grantRole(ROUTER_ROLE, address(router));
        binary.approvePoolToTransferTokenX();

        pool.grantRole(OPTION_ISSUER_ROLE, address(binary));
        // ensure pool has tokens
        token.approve(address(pool), type(uint256).max);
        pool.provide(10e6, 0);

        vm.stopPrank();
        // END Setup

        // Provide Alice and Bob with tokens
        uint256 amount = 1e6;
        vm.prank(owner);
        token.transfer(alice, amount);
        vm.prank(owner);
        token.transfer(bob, amount);

        // Alice creates a trade, this is the first trade so it's queueId should be 0
        vm.prank(alice);
        token.approve(address(router), type(uint256).max);
        vm.prank(alice);
        uint256 aliceQueueId = router.initiateTrade(
            amount, // uint256 totalFee,
            1 hours, // uint256 period,
            true, // bool isAbove,
            address(binary), // address targetContract,
            1250, // uint256 expectedStrike,
            5e2,// uint256 slippage,
            true, // bool allowPartialFill,
            "", // string memory referralCode,
            0 // uint256 traderNFTId
        );

        assertEq(aliceQueueId, 0);

        // Next, Bob creates a trade, queueId should be 1
        vm.prank(bob);
        token.approve(address(router), type(uint256).max);
        vm.prank(bob);
        uint256 bobQueueId = router.initiateTrade(
            amount, // uint256 totalFee,
            1 hours, // uint256 period,
            false, // bool isAbove,
            address(binary), // address targetContract,
            1250, // uint256 expectedStrike,
            5e2,// uint256 slippage,
            true, // bool allowPartialFill,
            "", // string memory referralCode,
            0 // uint256 traderNFTId
        );

        assertEq(bobQueueId, 1);

        // The keeper resolves Bob's trade first
        address asset = address(binary);
        uint256 timestamp = block.timestamp;
        uint256 price = 1250;
        bytes32 digest = ECDSA.toEthSignedMessageHash(
            keccak256(abi.encodePacked(timestamp, asset, price))
        );
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(publisherPrivateKey, digest);

        IBufferRouter.OpenTradeParams[] memory params = new IBufferRouter.OpenTradeParams[](1);
        params[0].queueId = bobQueueId;
        params[0].timestamp = timestamp;
        params[0].asset = asset;
        params[0].price = price;
        params[0].signature = abi.encodePacked(r,s,v);

        vm.prank(keeper);
        router.resolveQueuedTrades(params);

        // This will update nextQueueIdToProcess to 2, while Alice's trade is still queued
        assertEq(router.nextQueueIdToProcess(), 2);
    }
}

Tool used

None

Recommendation

This isn't a blocking issue since a keeper can still process older trades (i.e. trades with lower queue id than next to be processed). However, consider a) removing these variables since they aren't used in the contract and may lead to reporting inconsistent values or b) enforce a correct order so the values are accurate.