Foundation contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L556-L560

Vulnerability details

Impact

An Offer which is made for an NFT when auction has ended, but its winner hasn't received the NFT yet, can be stolen by this winner as _transferFromEscrow being called by _acceptOffer will transfer the NFT to the winner, finalising the auction, while no transfer to the user who made the offer will happen.

This way the auction winner will obtain both the NFT and the offer amount after the fees at no additional cost, at the expense of the user who made the offer.

Proof of Concept

When an auction has ended, there is a possibility to make the offers for an auctioned NFT as:

makeOffer checks _isInActiveAuction:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L200

_isInActiveAuction returns false when auctionIdToAuction[auctionId].endTime < block.timestamp, so makeOffer above can proceed:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L666-L669

Then, the auction winner can call acceptOffer -> _acceptOffer (or setBuyPrice -> _autoAcceptOffer -> _acceptOffer).

_acceptOffer will try to transfer directly, and then calls _transferFromEscrow:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L262-L271

If the auction has ended, but a winner hasn't picked up the NFT yet, the direct transfer will fail, proceeding with _transferFromEscrow in the FNDNFTMarket defined order:

function _transferFromEscrow(
address nftContract,
uint256 tokenId,
address recipient,
address seller
) internal override(NFTMarketCore, NFTMarketReserveAuction, NFTMarketBuyPrice, NFTMarketOffer) {
super._transferFromEscrow(nftContract, tokenId, recipient, seller);
}

NFTMarketOffer._transferFromEscrow will call super as nftContractToIdToOffer was already deleted:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L296-L302

NFTMarketBuyPrice._transferFromEscrow will call super as there is no buy price set:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketBuyPrice.sol#L283-L293

Finally, NFTMarketReserveAuction._transferFromEscrow will send the NFT to the winner via _finalizeReserveAuction, not to the user who made the offer:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L556-L560

The recipient user who made the offer is not present in this logic, the NFT is being transferred to the auction.bidder, and the original acceptOffer will go through successfully.

Recommended Mitigation Steps

An attempt to set a buy price from auction winner will lead to auction finalisation, so _buy cannot be called with a not yet finalised auction, this way the NFTMarketReserveAuction._transferFromEscrow L550-L560 logic is called from the NFTMarketOffer._acceptOffer only:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L270

is the only user of

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L550-L560

This way the fix is to update L556-L560 for the described case as:

Now:

// Finalization will revert if the auction has not yet ended.
_finalizeReserveAuction(auctionId, false);

// Finalize includes the transfer, so we are done here.
return;

To be, we leave the NFT in the escrow and let L564 super call to transfer it to the recipient:

// Finalization will revert if the auction has not yet ended.
_finalizeReserveAuction(auctionId, true);

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCore.sol#L77-L87

Vulnerability details

Impact

If a NFT happens to be in escrow with neither buyPrice, nor auction being initialised for it, there is a way to obtain it for free by any actor via makeOffer, acceptOffer combination.

I.e. a malicious user can track the FNDNFTMarket contract and obtain any NFT from it for which there are no buyPrice or auction structures initialised. For example, if a NFT is mistakenly sent to the contract, an attacker can immediately steal it.

This will happen as NFT is being guarded by buyPrice and auction structures only. The severity here is medium as normal usage of the system imply that either one of them is initialised (NFT was sent to escrow as a part of setBuyPrice or createReserveAuction, and so one of the structures is present), so this seems to leave only mistakenly sent assets exposed.

Proof of Concept

An attacker can make a tiny offer with makeOffer:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L189

Then call acceptOffer, which will lead to _acceptOffer.

Direct NFT transfer will fail in _acceptOffer as the NFT is being held by the contract and _transferFromEscrow will be called instead:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L262-L271

_transferFromEscrow calls will proceed according to the FNDNFTMarket defined order:

function _transferFromEscrow(
...
) internal override(NFTMarketCore, NFTMarketReserveAuction, NFTMarketBuyPrice, NFTMarketOffer) {
   super._transferFromEscrow(nftContract, tokenId, recipient, seller);
}

If there are no corresponding structures, the NFTMarketOffer, NFTMarketBuyPrice and NFTMarketReserveAuction versions of _transferFromEscrow will pass through the call to NFTMarketCore's plain transfer implementation:

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCore.sol#L77-L87

This will effectively transfer the NFT to the attacker, which will pay gas costs and an arbitrary small offer price for it.

Recommended Mitigation Steps

Consider adding additional checks to control who can obtain unallocated NFTs from the contract.

Protocol controlled entity can handle such cases manually by initial sender's request.