WATCHPUG - Bull can prevent `settleContract()`

[sherlock-admin]

WATCHPUG

high

Bull can prevent settleContract()

Summary

The bull can intentionally cause out-of-gas and revert the transaction and prevent settleContract().

Vulnerability Detail

As IERC721(order.collection).safeTransferFrom() is used in settleContract() which will call IERC721Receiver(to).onERC721Received() when the to address is an contract.

This gives the bull a chance to intentionally prevent the transaction from happening by consuming a lot of gas and revert the whole transaction.

Impact

The bear (victim) can not settleContract() therefore cannot exercise their put option rights. The bull (attacker) always wins.

Code Snippet

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L374-L411

Tool used

Manual Review

Recommendation

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);
}

[datschill]

PR fixing this issue : https://github.com/BullvBear/bvb-solidity/pull/14

[sherlock-admin]

Escalate for 5 USDC

Hey Hi, From all issues considered duplicates here, this current issue https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/111 and https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/100 are incorrect, and shouldn't be considered. Both of https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/111, https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/100 say that bulls can stop bears from settling via out-of-gas revert.

However, Bulls can not directly stop bears from settling, as these 2 reports depict. All they can do is increase transaction costs and add extra overhead, as correctly explained in

https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/147 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/18 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/142

Verification Copy follwoing test inside bvb-protocol> test> integration> and run forge test --match-contract TestGasGrief -vvvvv https://gist.github.com/abhishekvispute/26bc7e0e231d1ca3cf4deae2dd5d2ebf

In this test, you will see that the bull transfers the position to a malicious bull which has an infinite loop on received, but still bear can settle by paying additional overhead. Hence the issue is not that bulls can cause out-of-gas reverts but bulls adding additional gas overhead. The recommendation is also wrong for https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/111. Consider removing them from included issues.

test trace (check how it comes out of "out of gas revert" and still allows to settle)

[PASS] testGasGrief((uint256,uint256,uint256,uint256,uint256,uint16,address,address,address,bool)) (runs: 256, μ: 7301975, ~: 7304974)
Traces:
 .......
 .......
 .......
    ├─ [0] VM::prank(Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c]) 
    │   └─ ← ()
    ├─ [6672810] BvbProtocol::settleContract((4891, 864, 3601, 86401, 10, 20, 0x7B5969C684b101DA876186F1b8f3aB808e308B7c, 0xCe71065D4017F316EC606Fe4422e11eB2c47c246, 0x185a4dc360CE69bDCceE33b3784B0282f7961aea, false), 1234) 
    │   ├─ [6533561] BvbERC721::safeTransferFrom(Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], BvbMaliciousBull: [0x42997aC9251E5BB0A61F4Ff790E5B991ea07Fd9B], 1234) 
    │   │   ├─ emit Transfer(from: Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], to: BvbMaliciousBull: [0x42997aC9251E5BB0A61F4Ff790E5B991ea07Fd9B], id: 1234)
    │   │   ├─ [6522355] BvbMaliciousBull::onERC721Received(BvbProtocol: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382], Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], 1234, 0x) 
    │   │   │   └─ ← "EvmError: OutOfGas"
    │   │   └─ ← "EvmError: Revert"
    │   ├─ [23775] BvbERC721::safeTransferFrom(Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], BvbProtocol: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382], 1234) 
    │   │   ├─ emit Transfer(from: Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], to: BvbProtocol: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382], id: 1234)
    │   │   ├─ [864] BvbProtocol::onERC721Received(BvbProtocol: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382], Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], 1234, 0x) 
    │   │   │   └─ ← 0x150b7a02
    │   │   └─ ← ()
    │   ├─ [22852] BvbERC20::transfer(Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], 5755) 
    │   │   ├─ emit Transfer(from: BvbProtocol: [0xf5a2fE45F4f1308502b1C136b9EF8af136141382], to: Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c], amount: 5755)
    │   │   └─ ← true
    │   ├─ emit SettledContract(orderHash: 0xf737abeb07bf156db35b7a738422241312110236659091d877b6d09846af2e82, tokenId: 1234, order: (4891, 864, 3601, 86401, 10, 20, 0x7B5969C684b101DA876186F1b8f3aB808e308B7c, 0xCe71065D4017F316EC606Fe4422e11eB2c47c246, 0x185a4dc360CE69bDCceE33b3784B0282f7961aea, false))
    │   ├─ emit log_named_uint(key: safeTransferCase Gas, val: 107483)
    │   └─ ← ()
    ├─ [564] BvbERC20::balanceOf(Bull: [0xCf03Dd0a894Ef79CB5b601A43C4b25E3Ae4c67eD]) [staticcall]
    │   └─ ← 0
    ├─ [564] BvbERC20::balanceOf(Bear: [0x7B5969C684b101DA876186F1b8f3aB808e308B7c]) [staticcall]
    │   └─ ← 5755
    └─ ← ()

You've deleted an escalation for this issue.

[jack-the-pug]

Fix confirmed