NFT attacks

In this article, I have organized the common NFT vulnerabilities that I have identified during previous audits on Solodit. It may not cover all possible vulnerabilities, but I have made my best effort to include as many as possible.

Use safe functions instead of mint, transferFrom for ERC721

There are certain smart contracts that do not support ERC721, using transferFrom() , mint() may result in the NFT being sent to such contracts. As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Here is a code example from a contest with this issue
mint(msg.sender, tokenId);

  function _stakeToken(uint _tokenId, uint _unlockTime) internal returns (uint) {
    if (_unlockTime > 0) {
      unlockTime[_tokenId] = _unlockTime;
      uint fullStakedTimeBonus = ((_unlockTime - block.timestamp) * stakingSettings.maxStakeBonusAmount) / stakingSettings.maxStakeBonusTime;
      stakedTimeBonus[_tokenId] = _tokenId < 10000 ? fullStakedTimeBonus : fullStakedTimeBonus / 2;
    }

    // Transfer the underlying token from the owner to this contract
    IERC721 collection;
    if (_tokenId < 10000) {
      collection = frankenpunks;
      stakedFrankenPunks++;
    } else {
      collection = frankenmonsters;
      stakedFrankenMonsters++;
    }

    address owner = collection.ownerOf(_tokenId);
    if (msg.sender != owner && !collection.isApprovedForAll(owner, msg.sender) && msg.sender != collection.getApproved(_tokenId)) revert NotAuthorized();
    collection.transferFrom(owner, address(this), _tokenId);

    // Mint the staker a new ERC721 token representing their staked token
    _mint(msg.sender, _tokenId);

    // Return the voting power for this token based on staked time bonus and evil score
    return getTokenVotingPower(_tokenId);
  }

Other similar examples from Solodit: 1 2 3 4 5 6 7 8 9

onERC721Received reenterency

Whenever you see that nft is transfering, check if that code is following checks-effects pattern or had reentrency guard otherwise project might be vulnrabale to reentrency exploit due to NFT onERC721Received hook. Let's examine code from a recent issue

  function mint(
        MintTrade memory _mintTrade
    ) external onlyMinter {
        uint newTokenID = _tokenIds.current();

        Trade storage newTrade = _trades[newTokenID];
        newTrade.margin = _mintTrade.margin;
        newTrade.leverage = _mintTrade.leverage;
        newTrade.asset = _mintTrade.asset;
        newTrade.direction = _mintTrade.direction;
        newTrade.price = _mintTrade.price;
        newTrade.tpPrice = _mintTrade.tp;
        newTrade.slPrice = _mintTrade.sl;
        newTrade.orderType = _mintTrade.orderType;
        newTrade.id = newTokenID;
        newTrade.tigAsset = _mintTrade.tigAsset;

        _safeMint(_mintTrade.account, newTokenID);   // make external call because of safeMint() usage
        if (_mintTrade.orderType > 0) { // update the values of some storage functions
            _limitOrders[_mintTrade.asset].push(newTokenID);
            _limitOrderIndexes[_mintTrade.asset][newTokenID] = _limitOrders[_mintTrade.asset].length-1;
        } else {
            initId[newTokenID] = accInterestPerOi[_mintTrade.asset][_mintTrade.tigAsset][_mintTrade.direction]*int256(_mintTrade.margin*_mintTrade.leverage/1e18)/1e18;
            _openPositions.push(newTokenID);
            _openPositionsIndexes[newTokenID] = _openPositions.length-1;

            _assetOpenPositions[_mintTrade.asset].push(newTokenID);
            _assetOpenPositionsIndexes[_mintTrade.asset][newTokenID] = _assetOpenPositions[_mintTrade.asset].length-1;
        }
        _tokenIds.increment();
    }

As you can see code make external call to attacker specified address(sendTo) by collateral.addr.safeTransferFrom(address(this), sendTo, collateral.id) which would call sendTo address onERC721Received() function and then code checks to make sure that debt is lower than max debt and code retrieves the updated debt after the call to give user a chance to pay some of the debt in the hook function.

Other examples from Solodit: 1 2 3 4 5

Owner control NFT after transferring, burning etc

Sometimes, developers forget that the owner of an NFT can still utilize the NFT's power even after transferring it. Similar issues can arise when OpenSea fails to cancel sale offers upon NFT transfer. If the NFT is eventually transferred back to the original owner, all previous approvals remain intact, which may come as an unexpected outcome for the owner. Let's examine code from a recent issue in the Sherlock contest. In this case, there is a football club (represented as an NFT) that can have multiple players (also represented as NFTs) and funds (ERC-20 tokens). The owner has the ability to set the football club's players and funds to themselves using these functions.

function setApprovalForERC20(
    IERC20 erc20Contract,
    address to,
    uint256 amount
) external onlyClubOwner {
    erc20Contract.approve(to, amount);
}

/**
 * @notice Sets approval for ERC721 tokens.
 * @param erc721Contract ERC721 contract address.
 * @param to The address of token spender.
 * @param approved Boolean flag indicating whether approved or not.
 * @dev only the club owner address allowed.
 */
function setApprovalForERC721(
    IERC721 erc721Contract,
    address to,
    bool approved
) external onlyClubOwner {
    erc721Contract.setApprovalForAll(to, approved);
}

but there is no implementation of removing those approvals on transfer, which means that the previous owner can still transfer the club's players and funds to themselves after selling the club.

Other similar examples from Solodit: 1 2 3 4 5 6 7 8 9

Missing ownerOf whenever function arguments contain arbitrary NFT ID

Whenever there is an arbitrary NFT ID inside function arguments, there should be checks to ensure that the caller is the owner of the NFT on which this function operates. Let's examine code from a recent issue in the Sherlock contest

function withdrawFromGauge(uint256 _NFTId, address[] memory _tokens)  public  {
    uint256 amount = depositReceipt.pooledTokens(_NFTId);
    depositReceipt.burn(_NFTId);
    gauge.getReward(address(this), _tokens);
    gauge.withdraw(amount);
    //AMMToken adheres to ERC20 spec meaning it reverts on failure, no need to check return
    //slither-disable-next-line unchecked-transfer
    AMMToken.transfer(msg.sender, amount);
}

This function is used to withdraw funds from the gauge by burning the deposit receipt NFT. After the NFT is either transferred or approved, a malicious user could withdraw the NFT for themselves.

Other similar examples from Solodit: 1 2

The protocol doesn't follow the EIP standard

Smart people wrote EIP standards with a lot of effort on the security, projects that are not compliant with standards might be vulnerable. Let's examine code from a contest

  function approve(address to, uint256 tokenId) external payable {
    address tokenOwner = _tokenOwner[tokenId];
    require(to != tokenOwner, "ERC721: cannot approve self");
    require(_isApproved(msg.sender, tokenId), "ERC721: not approved sender");
    if (_isEventRegistered(HolographERC721Event.beforeApprove)) {
      require(SourceERC721().beforeApprove(tokenOwner, to, tokenId));
    }
    _tokenApprovals[tokenId] = to;
    emit Approval(tokenOwner, to, tokenId);
    if (_isEventRegistered(HolographERC721Event.afterApprove)) {
      require(SourceERC721().afterApprove(tokenOwner, to, tokenId));
    }
  }

Bob calls approve to approve Alice on token ID 42 (which is owned by Bob). One week later, Bob sees that a malicious address was approved for his token ID 42 (e.g. because Alice got phished) and stole his token. Bob wonders how this is possible because Alice should not have permission to approve other addresses. However, because the protocol did not follow EIP-721, it was possible.

Other similar examples from Solodit: 1 2 3 4 5 6

Counter for NFT is not being tracked/updated correctly

There should be some symmetries for different functions. For instance, a burn function should (usually) undo all the state changes of a mint function. Asymmetries in these function pairs (e.g., forgetting to unset a field or to subtract from a value) will to overwriting/assigning existing nft to other users. Let's examine code from this issue

    function register(uint256 _cidNFTID) external {
43:         if (ERC721(cidNFT).ownerOf(_cidNFTID) != msg.sender)
44:             // We only guarantee that a CID NFT is owned by the user at the time of registration
45:             // ownerOf reverts if non-existing ID is provided
46:             revert NFTNotOwnedByUser(_cidNFTID, msg.sender);
47:         cidNFTs[msg.sender] = _cidNFTID;
48:         emit CIDNFTAdded(msg.sender, _cidNFTID);
49:     }

The same CID NFT can be registered under multiple accounts at the same time.

This happens because the registration process doesn't check if the CID NFT has been previously registered in order to blank the previous association. This may lead to having the same CID NFT registered for multiple accounts at the same time, as the CID NFT can be registered, then transferred, then registered again, and this process can be repeated any number of times.

Other examples from Solodit: 1 2 3

ERC-777 tokens

In all sherlock's contests, there is a question for project owners

Which ERC777 tokens do you expect will interact with the smart contracts?

This means that if the project supports it then there will be definitely some issues with it. ERC-777 tokens have been exploited multiple times in the past. I suggest searching for ERC-777 on Solodit to see why.

About 0xVolodya

0xVolodya is an independent smart contract security researcher. Ranked #2 out of 343 on the 60-day leaderboard and ranked #5 out of 530 on the 90-day leaderboard at code4rena. Currently available for projects. dm me on Twitter 0xVolodya.