Closed sherlock-admin closed 1 year ago
PR fixing this issue : https://github.com/BullvBear/bvb-solidity/pull/1
PR fixing the reentrancy problem : https://github.com/BullvBear/bvb-solidity/pull/2
PR fixing the transfer to 0x0 : https://github.com/BullvBear/bvb-solidity/pull/3
hansfriese
high
Attacker can trigger permanent lock of funds of normal traders
Summary
An attacker can buy bull positions using a custom contract with
onERC721Received
callback and transfer his position to a zero address on the settlement or reclaiming. This makes that order stillmatchable
because it passes thecheckIsValidOrder
. This can lead to potential loss for the order maker and the future taker, and their funds will be locked in the protocol.Vulnerability Detail
The protocol uses
safeTransferFrom
to send NFTs to the bull and it is possible for the bull to do additional things in theIERC721Receiver.onERC721Received
callback. Although the major functions are written asnonReentrant
, there are still public functions that can be called from the callback and it is possible for an attacker to manipulate the protocol storage.The scenario would be:
IERC721Receiver
and in theonERC721Received
function makes a call totransferPosition
withrecipient=0
.settledContracts[contractId]
is set to true andbulls[contractId] = address(0)
(because of the callback).reclaimedContracts[contractId]
is set to true andbulls[contractId] = address(0)
(because of the callback).Note that the attacker set
bulls[contractId] = address(0)
. This makes it possible for a future taker to match that order again becausecheckIsValidOrder
function does not checksettledContracts
orreclaimedContracts
but onlybulls[uint(orderHash)] == address(0)
. But that order was already settled or reclaimed and none of the taker or the maker can claim their funds back.Of course this is only possible if the order maker didn't change the nounce yet and approves the new order match but it is very likely for normal traders to approve the protocol for transfering funds with a belief that the order will be verified.
Impact
A sophisticated attacker can exploit using the ERC721 callback and make \<invalid> orders with abnormal status. Because the trader's funds can be locked in the protocol permanently, I rate the impact as high.
Code Snippet
https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L394 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L521 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L734
Tool used
Manual Review
Recommendation
checkIsValidOrder
ortransferPosition
asnonReentrant
.Duplicate of #114