search

code-423n4 / 2021-04-meebits-findings

0 stars 0 forks source link

Arbitrary Transfer of Unowned NFTs #4

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xsomeone

Vulnerability details

Impact

Due to the way the market functions are structured, it is possible to arbitrarily transfer any NFT that is not owned by any address.

Proof of Concept

The function in question is the tradeValid function which is invoked by acceptTrade before the trade is performed. It in turn validates the signature of a trade via verify (https://github.com/code-423n4/2021-04-redacted/blob/main/Beebots.sol#L556-L576) which does not account for the behaviour of ecrecover.

When ecrecover is invoked with an invalid signature, the zero-address is returned by it meaning that verify will yield true for the zero-address as long as the signature provided is invalid.

This can be exploited to transfer any NFT whose idToOwner is zero, including NFTs that have not been minted yet.

Tools Used

Manual review.

Recommended Mitigation Steps

I advise an additional check to be imposed within verify that ensures the signer is not the zero-address which will alleviate this check. For more details, consult the EIP721 implementation by OpenZeppelin (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/cryptography/ECDSA.sol#L53-L71).

pauliax commented 3 years ago

I do not understand why this was assigned as a high-risk issue. I was also wrapping my head around this but in the end, decided not to report it as I was not able to come up with a possible attack vector. Here is why: function verify is only invoked once with offer.maker passed as the signer so for it to work you must pass maker as 0x0 address. When function acceptTrade continues and invokes _transfer on makerIds this, in turn, calls _removeNFToken but as ownerToIds of 0x0 is 0, SafeMath will fail here:

   uint256 lastTokenIndex = ownerToIds[_from].length.sub(1);

Can someone explain if I am missing something and is it somehow possible to exploit this? @dangerousfood