Scanning for vulnerable ERC721 implementations

An ERC (Ethereum Request for Comments) standard is a de-facto guide to describe interfaces, formats, and procedures for the smart contracts in the Ethereum ecosystem. Although the specifications are mostly precise and accurate, there is still room for ambiguities and misinterpretations. Moreover, some tokens actually do not follow these specs completely as ERCs only describe behaviors, but do not enforce them. Anyone can create a smart contract that is ERC compliant on the surface: it has correct return types, emits appropriate events, and so on, but will it behave as expected? It's up to the contract developer and as such any interaction with a contract should be double-checked.

There are smart contract libraries that are battle-tested and used by almost every developer. Of course, this is about OpenZeppelin contracts that are the most popular in Ethereum and EVM compatible blockchains. However, sometimes developers may find functionality in popular libraries not enough or not suitable. For instance, gas consumption is critical and contracts have to be heavily optimized. OZ contracts strive for better readability rather than saving gas, that is why custom "unofficial" implementations have come up. One example is the ERC721A contract created by developers of well-known Azuki NFTs. It is aimed at more efficient NFT minting, and there are no known bugs yet.

if you want to add additional features, source code forking is a common practice, it saves time compared to writing contracts from scratch. However, once you fork you have to maintain the codebase on your own and some subtle unpredictable bugs may arise because of the lack of full understanding of the code.  This was the case with the NFT project called "Distortion Genesis" and was highlighted by the BlockSec team when a malicious actor exploited the bug in custom transferFrom implementation forked from ERC721A. The vulnerability basically allowed to transfer anyone's NFTs because the access control block that checked msg.sender was removed:

function _transfer(
    address from,
    address to,
    uint256 tokenId
  ) internal virtual {
    TokenOwnership memory prevOwnership = ownershipOf(tokenId);

    require(
      prevOwnership.addr == from,
      "ERC721A: transfer from incorrect owner"
    );
    require(to != address(0), "ERC721A: transfer to the zero address");
    /* ... */
  }

Compare with the original ERC721A:

function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) private {
        TokenOwnership memory prevOwnership = ownershipOf(tokenId);

        bool isApprovedOrOwner = (_msgSender() == prevOwnership.addr ||
            getApproved(tokenId) == _msgSender() ||
            isApprovedForAll(prevOwnership.addr, _msgSender()));

        require(isApprovedOrOwner, 'ERC721A: transfer caller is not owner nor approved');

        require(prevOwnership.addr == from, 'ERC721A: transfer from incorrect owner');
        require(to != address(0), 'ERC721A: transfer to the zero address');
        /* ... */
  }

We are not quite sure if it was a deliberate malicious intention from the developers, but we decided to find smart contracts with a similar vulnerability. This is really easy with semgrep which allows to parse and scan large codebases using flexible rules. We wanted to find every _transfer function declaration that checks prevOwnership.addr (a marker of ERC721A contracts) but doesn't have any checks on msg.sender. As a result, we have come up with the following semgrep rule:

rules:
 -
    id: erc721-arbitrary-transferfrom
    message: Custom ERC721 implementation lacks access control checks in _transfer()
    metadata:
        references:
        - https://twitter.com/BlockSecAlert/status/1516289618605654024
        - https://etherscan.io/address/0xf3821adaceb6500c0a202971aecf840a033f236b
        category: access-control
        tags:
        - distortion-genesis
    patterns:
      - pattern-inside: |
         function _transfer(...) {
         ...
         }
      - pattern-inside: |
         require(prevOwnership.addr == $FROM, ...);
         ...
      - pattern-not-inside: |
         (<... _msgSender() == $FROM ...>);
         ...
      - pattern-not-inside: |
         (<... msg.sender == $FROM ...>);
         ...
      - pattern-not-inside: |
         require(_isApprovedOrOwner(...);
         ...
      - pattern: _approve(...);
    languages: 
    - solidity
    severity: WARNING

A quick scan of Ethereum mainnet smart contracts from smart-contract-sanctuary yielded two results. One was the aforementioned "Distortion Genesis" smart contract and the other was also an ERC721A fork but a bit different one. This contract looked active with a balance of more than $270000.

The code looked indeed vulnerable so we decided to simulate a transaction on Tenderly that would call transferFrom with a random NFT holder address and an address of ours as a destination. 

Aaaand it worked! But what is worse, the contract had the following function that allowed to refund unwanted NFTs.

function refund(uint256 tokenId) external callerIsUser nonReentrant {
    require(paused == false, "Contract paused.");
    require(refundList[msg.sender] == true, "Not allowed to whitelist mint.");
    require(ownerOf(tokenId) == msg.sender, "You are not the owner!");
    require(block.timestamp - mintedTime[tokenId] <= 604800, "The time for return has expired!");

    safeTransferFrom(msg.sender, address(this), tokenId);
    payable(msg.sender).transfer(price);
}

The function transfers the NFT from the user to the contract itself and sends back the original cost in ether. It is possible that the developer deleted the approval block with access control checks in _transfer just to save gas for the users who didn't want to send an additional approval transaction so that the contract could transfer the NFT from a user. A careless developer might have assumed that require(prevOwnership.addr == from,"ERC721A: transfer from incorrect owner"); is enough. But we never know.

What is important is that this code basically allowed to drain all the funds from the smart contract. One just needed to repeatedly call transferFrom to get the NFT back from the smart contract address to their address, and call refund.

We quickly contacted the developers, they acknowledged the bug but, to our surprise, were not so fast to fix it. Instead, they decided just to wait for 7 days when refunds will no longer be possible.

This time they were lucky enough and nobody noticed the bug for 7 days. Later the contract owner withdrew the funds and was no longer reachable in DMs on Twitter. With that said, we can finally publish the rule to find vulnerable transferFrom in ERC721 forks in the semgrep-smart-contracts repository.


If you need a professional security audit for your blockchain software, smart contracts or a dApp, do not hesitate to submit the contact form. Check out our previous audits at GitHub