A warm welcome, arcanists! I’m Xuannü, the WITCH of WILL and keeper of the coven at Crypto Coven.

As our anointed blockchain whisperer, I wrote the ERC-721 contract for the genesis series of 9,999 WITCHES, aided by our acolytes Oizys and Enthea and audited by our Solidity adeptus Mersenne.

We’ve been deeply gratified and (honestly) shocked to receive a lot of praise for our first foray into web3 development, and we’re quite proud of what we’ve been able to do with so little experience—we have a deeper dive into the contract as a whole in the works, coming soon.

However, for newer Solidity developers using the Crypto Coven contract as a reference, we wanted to pay it forward by sharing two bugs we’ve discovered in our contract and providing examples of correct implementations.

The Return of the Void Witch: Total Mintable WITCHES

Oh, gifting logic. A painfully simple but subtle bug in the gifting logic had our first smart contract grinding to a halt, and in our redeployed contract, we found ourselves plagued once again by a different gifting bug.

We wrote this modifier canMintWitches() to check if addresses should be able to mint more WITCHES in the public sale:

uint256 public maxWitches; // initialized to 9,999
uint256 public maxGiftedWitches; // initialized to 250

modifier canMintWitches(uint256 numberOfTokens) {
    require(
        tokenCounter.current() + numberOfTokens <=
            maxWitches - maxGiftedWitches,
        "Not enough witches remaining to mint"
    );
    _;
}

What’s the bug? It’s a difficult one to spot because it’s only a bug given certain conditions.

The problem is that there should be 9,749 WITCHES available to mint in the public function and 250 reserved to mint by the owner-only function, for a total of 9,999. This logic works perfectly if no WITCHES are gifted until the public sale is sold out.

However, we had always intended to gift WITCHES throughout the mint, which means that the right side of that check should have been dynamic. As the number of gifted WITCHES increased, the allowable highest token ID for witches in the public sale should have increased as well.

At the time that the public sale closed, we had gifted 93 WITCHES. That meant that tokenCounter.current() had reached 9,749, ending the open mint period—but only 9,656 of them were WITCHES minted in the community and public sales.

The canGiftWitches() function, however, intentionally limited the number of WITCHES we could gift to 250, which meant that we couldn’t circumvent the bug that way, either:

uint256 public maxWitches; // initialized to 9,999
uint256 public maxGiftedWitches; // initialized to 250
uint256 private numGiftedWitches;

modifier canGiftWitches(uint256 num) {
    require(
        numGiftedWitches + num <= maxGiftedWitches,
        "Not enough witches remaining to gift"
    );
    require(
        tokenCounter.current() + num <= maxWitches,
        "Not enough witches remaining to mint"
    );
    _;
}

As a result, 93 WITCHES are lost to the void—this contract will only ever mint 9,906 in total.

(For anyone curious about the fate of the WITCHES trapped somewhere in the ether: stay attuned, for these WITCHES may yet find their way back to the weird wilds...)

Fixing the Implementation

This bug is easily remedied in our contract by adding numGiftedWitches, a variable we already store, to the check:

uint256 public maxWitches; // initialized to 9,999
uint256 public maxGiftedWitches; // initialized to 250
uint256 private numGiftedWitches;

modifier canMintWitches(uint256 numberOfTokens) {
    require(
        tokenCounter.current() + numberOfTokens <=
            maxWitches - maxGiftedWitches + numGiftedWitches,
        "Not enough witches remaining to mint"
    );
    _;
}

A Sealed Vault: Royalties

A big thanks to Zach Burks (founder of Mintable and one of the authors of EIP-2981) for reporting this bug.

It was important to us to have on-chain royalties, not just use platform-specific, off-chain implementations, which is what led us to EIP-2981. The code to support the standard was simple:

function royaltyInfo(uint256 tokenId, uint256 salePrice)
    external
    view
    override
    returns (address receiver, uint256 royaltyAmount)
{
    require(_exists(tokenId), "Nonexistent token");

    return (address(this), SafeMath.div(SafeMath.mul(salePrice, 5), 100));
}

How does it work? Marketplaces call the function to read data for a receiver address and a royaltyAmount, then send the royalties accordingly. In our case, our receiver is the contract address, and the royalty amount is 5%. Easy enough.

Except, crucially, as of Solidity 0.6.x, smart contracts cannot receive Ether directly without implementing a receive() function, which we didn’t have.

While our tests checked that the royaltyInfo() function returned the correct values, we never tested receiving royalties to the contract, so transactions from marketplaces that attempted to send us royalties would trigger a revert.

The remedy for us, in this case, was luckily pretty simple, thanks to the Royalty Registry. We configured an override pointing to a different receiver address (in this case, our multi-sig wallet), so now marketplaces that read from the Royalty Registry will use the override.

Fixing the Implementation

The easiest way to fix this bug to support EIP-2981 is to simply return the same owner address that receives withdrawals, rather than the contract address. Another option would be to add a royaltyReceiverAddress variable and a setter to make this value configurable.

If you do want to receive Ether to the contract address, all you need to do is add a receive() function to the contract:

receive() external payable {}

Coda

Learning to develop in Solidity can be a trial by fire—mistakes, minor and major, live eternally ensconced on the blockchain, often at great cost. But the rigid, unforgiving nature of the space can have its own kind of magic, in the creativity born out of constraint and the solidarity forged through shared sleepless nights. To any budding arcanists making their own way through the wilds: I hope that this offering of our learnings further illuminates the path.

If you stumble upon any other bugs in our contract, please DM me on Twitter—we intend to keep this post up-to-date!