The transferPosition() function does not check if the recipient is a contract

Summary

The bull can transfer his position for free with the transferPosition() function, the problem is that the function does not check the recipient.

Vulnerability Detail

The transferPosition() function does not check the recipient if it is a contract or not.

Impact

If the transferPosition() function transfers to a contract which does not have onERC721Received function, the recipient contract will not able to withdrawToken() the NFT. More serious if the contract does not have an upgrade pattern.

Test:

// SettleContract.t.sol
// I created a contract (naivecontract) with no functions.
function test_oxbepresent_transfer_to_naivecontract_cannot_withdraw_NFT() public {
    BvbProtocol.Order memory order = defaultOrder();
    bytes32 orderHash = bvb.hashOrder(order);

    bytes memory signature = signOrder(bullPrivateKey, order);
    bvb.matchOrder(order, signature);

    // Transfer position to naive contract
    vm.prank(bull);
    bvb.transferPosition(orderHash, true, address(naiveusercontract));
    vm.stopPrank();

    // the NFT will be in the BVB Protocol
    bvb.settleContract(order, tokenIdBear);
    assertEq(doodles.ownerOf(tokenIdBear), address(bvb), "NFT should have been transfered to BVB Protocol");

    // Wtihdraw the NFT but the naive contract will does not have the onERC721Received function
    // so the NFT will be stuck in BVB Protocol
    vm.expectRevert();
    bvb.withdrawToken(orderHash, tokenIdBear);
}

Code Snippet

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

function transferPosition(bytes32 orderHash, bool isBull, address recipient) public {
    // ContractId
    uint contractId = uint(orderHash);

    if (isBull) {
        // Check that the msg.sender is the Bull
        require(msg.sender == bulls[contractId], "SENDER_NOT_BULL");

        bulls[contractId] = recipient;
    } else {
        // Check that the msg.sender is the Bear
        require(msg.sender == bears[contractId], "SENDER_NOT_BEAR");

        bears[contractId] = recipient;
    }

    emit TransferedPosition(orderHash, isBull, recipient);
}

Tool used

VisualStudio/Foundry

Recommendation

The documentation says the transfer is to another wallet. So the transferPosition() function can check that transfer to contracts are not allowed.

sherlock-audit / 2022-11-bullvbear-judging

0xbepresent - The transferPosition() function does not check if the recipient is a contract #138