CryptoPunks NFTs may be stolen via deposit frontrunning

Summary

Depositing of CryptoPunks NFTs may be front run, a malicious actor may deposit someone else's CryptoPunks NFT.

Vulnerability Detail

Due to the CryptoPunks NFT collection not implementing the ERC721 standard, depositing of CryptoPunks NFTs is implemented via a direct sale:

token owner needs to call offerPunkForSaleToAddress and set the toAddress value to the address of the pool the token will be deposited to;
token owner then calls the addCollateral function of the ERC721 pool;
the pool buys the token from its owner.

However, addCollateral can be called by anyone: the pool will buy the token and will deposit it on the caller's account even if the caller is not the owner of the token.

Impact

CryptoPunks NFTs owner may lose their NFTs when trying to deposit them to an ERC721 pool. A malicious actor may front run the depositing and deposit the NFTs to their account. The malicious actor may then withdraw the NFTs.

Code Snippet

ERC721Pool.sol#L577 CryptoPunksMarket:

function offerPunkForSaleToAddress(uint punkIndex, uint minSalePriceInWei, address toAddress) {
    if (!allPunksAssigned) throw;
    if (punkIndexToAddress[punkIndex] != msg.sender) throw;
    if (punkIndex >= 10000) throw;
    punksOfferedForSale[punkIndex] = Offer(true, punkIndex, msg.sender, minSalePriceInWei, toAddress);
    PunkOffered(punkIndex, minSalePriceInWei, toAddress);
}

function buyPunk(uint punkIndex) payable {
    if (!allPunksAssigned) throw;
    Offer offer = punksOfferedForSale[punkIndex];
    if (punkIndex >= 10000) throw;
    if (!offer.isForSale) throw;                // punk not actually for sale
    if (offer.onlySellTo != 0x0 && offer.onlySellTo != msg.sender) throw;  // punk not supposed to be sold to this user
    if (msg.value < offer.minValue) throw;      // Didn't send enough ETH
    if (offer.seller != punkIndexToAddress[punkIndex]) throw; // Seller no longer owner of punk

    address seller = offer.seller;

    punkIndexToAddress[punkIndex] = msg.sender;
    balanceOf[seller]--;
    balanceOf[msg.sender]++;
    Transfer(seller, msg.sender, 1);

    punkNoLongerForSale(punkIndex);
    pendingWithdrawals[seller] += msg.value;
    PunkBought(punkIndex, msg.value, seller, msg.sender);

    // Check for the case where there is a bid from the new owner and refund it.
    // Any other bid can stay in place.
    Bid bid = punkBids[punkIndex];
    if (bid.bidder == msg.sender) {
        // Kill bid and refund value
        pendingWithdrawals[msg.sender] += bid.value;
        punkBids[punkIndex] = Bid(false, punkIndex, 0x0, 0);
    }
}

Tool used

Manual Review

Recommendation

Before buying a CryptoPunks NFT, consider checking that msg.sender is the owner of the token. For example:

diff --git a/contracts/src/ERC721Pool.sol b/contracts/src/ERC721Pool.sol
index b1bf36b..a512a9d 100644
--- a/contracts/src/ERC721Pool.sol
+++ b/contracts/src/ERC721Pool.sol
@@ -574,6 +574,7 @@ contract ERC721Pool is FlashloanablePool, IERC721Pool {
                 ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transferFrom(msg.sender ,address(this), tokenId);
             }
             else{
+                require(ICryptoPunks(_getArgAddress(COLLATERAL_ADDRESS)).punkIndexToAddress(tokenId) == msg.sender);
                 ICryptoPunks(_getArgAddress(COLLATERAL_ADDRESS)).buyPunk(tokenId);
             }

sherlock-audit / 2023-01-ajna-judging

Jeiwan - CryptoPunks NFTs may be stolen via deposit frontrunning #140