A Case Against Inheritance in Smart Contracts

In this article, I’ll attempt to persuade you to reduce your use of inheritance in smart contracts and to increase your skepticism when you see it.

The position that inheritance is to be avoided is, at least to some extent, one of personal preference. I won’t attempt to prove to you that inheritance is bad, but I will show you two examples that will hopefully nudge you in that direction.

Clarity is paramount

Programs must be written for people to read, and only incidentally for machines to execute.

—Harold Abelson, Structure and Interpretation of Computer Programs

This quote from 1984 long predates the blockchain, but it’s especially true when it comes to smart contracts. Smart contracts make it possible for people to engage in all sorts of transactions without having to trust one another. Trust between participants is replaced by trust that the smart contract does what it’s supposed to.

In Upgradeability Is a Bug, I argued that it’s impossible to trust a mutable smart contract because what it does may change. Similarly, it’s impossible to trust an inscrutable smart contract because what it does is too hard to determine in the first place.

Inheritance is one way to make smart contracts harder to understand. This is a problem for people who read the code and for the developers who need to maintain it.

Inheritance is hard for readers

This short example relies heavily on inheritance. See if you can spot the vulnerability:

pragma solidity 0.5.10;

contract Admin {
    address admin = msg.sender;

    function isAdmin() internal view returns (bool) {
        return msg.sender == admin;
    }
}

// Support adding extra admins.
contract MultiAdmin is Admin {
    mapping(address => bool) extraAdmins;

    function addAdmin(address who) external {
        require(isAdmin());
        extraAdmins[who] = true;
    }
    function isAdmin() internal view returns (bool) {
        return extraAdmins[msg.sender] || super.isAdmin();
    }
}

// Support permanently disabling admin functionality.
contract TempAdmin is Admin {
    bool administratable = true;
    function disableAdmin() external {
        require(isAdmin());
        administratable = false;
    }
    function isAdmin() internal view returns (bool) {
        return administratable && super.isAdmin();
    }
}

// To start with, only admins can deposit, and they can selfdestruct
// the contract if needed to recover from bugs. Once the testing
// phase is over, an admin will call disableAdmin(), and then the
// bank is open for business.
contract Bank is TempAdmin, MultiAdmin {
    mapping(address => uint256) public balanceOf;

    function deposit() external payable {
        if (administratable) {
            require(isAdmin(), "Admins only during testing.");
        }
        balanceOf[msg.sender] += msg.value;
    }

    function withdraw() external {
        uint256 amount = balanceOf[msg.sender];
        balanceOf[msg.sender] = 0;
        msg.sender.transfer(amount);
    }
    function kill() external {
        require(isAdmin());
        selfdestruct(msg.sender);
    }
}

After calling disableAdmin, no one should be able to call kill successfully, but it turns out they can. This vulnerability could be an innocent mistake, but it’s more likely intentional deception by the author, who can now run off with everyone’s ether.

The problem relates to multiple inheritance. Bank inherits from both TempAdmin and MultiAdmin. When the kill function calls isAdmin, which contract’s version of that function is executed? When that contract subsequently calls super.isAdmin, what happens?

The Solidity compiler uses an algorithm called C3 linearization to answer these questions. In this specific example, the code is executed in this order:

kill → MultiAdmin.isAdmin → TempAdmin.isAdmin → Admin.isAdmin

This is a bad order, leading to checking the equivalent of this:

extraAdmin[msg.sender] || (administratable && msg.sender == admin)

What we would prefer, and what happens if you write is MultiAdmin, TempAdmin (swapping the two base classes), is this order:

kill → TempAdmin.isAdmin → MultiAdmin.isAdmin → Admin.isAdmin

This results in correct code that checks the equivalent of this:

administratable && (extraAdmin[msg.sender] || msg.sender == admin)

This example was heavily inspired by Phil Daian’s entry in the 1st Underhanded Solidity Coding Contest. If you’d like a deeper understanding of the issue and a more devious smart contract example, I encourage you to read his writeup and code in full.

Inheritance is hard for developers

Consider the following contract, which implements a marketplace for ERC721 non-fungible tokens:

pragma solidity 0.5.10;

interface IERC721 {
    function transferFrom(address from, address to, uint256 tokenId) external;
}

contract NFTMarket {
    struct Listing {
        address seller;
        IERC721 token;
        uint256 tokenId;
        uint256 price;
    }

    Listing[] public listings;

    function list(
        IERC721 token,
        uint256 tokenId,
        uint256 price
    )
        public
    {
        listings.push(Listing({
           seller: msg.sender,
           token: token,
           tokenId: tokenId,
           price: price
        }));
    }

    function buy(uint256 listingId) public payable {
        Listing memory listing = listings[listingId];

        require(listing.seller != address(0), "No such listing.");
        require(msg.value == listing.price, "Incorrect payment.");

        delete listings[listingId];
        listing.token.transferFrom(
            listing.seller, msg.sender, listing.tokenId);
    }

    function cancel(uint256 listingId) public {
        require(msg.sender == listings[listingId].seller,
            "Only seller can cancel.");

        delete listings[listingId];
    }
}

This marketplace is perfectly functional, but perhaps the NFT market is volatile, and users would like the ability to set expiration dates on their listings. The following contract inherits from NFTMarket and adds expiration dates:

contract NFTMarketWithExpiration is NFTMarket {
    uint256[] public expirations;

    function list(
        IERC721 token,
        uint256 tokenId,
        uint256 price,
        uint256 expiration
    )
        public
    {
        super.list(token, tokenId, price);
        expirations.push(expiration);
    }

    function buy(uint256 listingId) public payable {
        require(now < expirations[listingId],
            "Listing has expired.");
        super.buy(listingId);
        delete expirations[listingId];
    }

    function cancel(uint256 listingId) public {
        super.cancel(listingId);
        delete expirations[listingId];
    }
}

I couldn’t modify the original contract’s Listing struct, so I’ve added a new array to keep track of expirations. I had to override all the functions, and readers have to jump back and forth between the base class and the derived class to understand what’s happening.

Did you spot the bug? I completely missed it in the first draft of this article, and inheritance is all I was thinking about! Because my new list function takes an extra parameter, it’s not actually overriding the original. I need this too:

function list(
    IERC721 token,
    uint256 tokenId,
    uint256 price
)
    public
{
    revert("Must pass an expiration.");
}

Without this override, callers will still be able to invoke the base contract’s list function, and the listing array will get out of sync with the expirations array. (Exactly what would happen is left as an exercise to the reader, but suffice it to say that the contract would be thoroughly broken.)

The derived contract is hard to read and hard to maintain. Depending on how you format the code, the new contract increases our total line count by about 75%.

Copy/paste/modify is better

Now I’ll take a different approach. Instead of inheriting from NFTMarket, I’ll copy/paste the code and directly modify it. In the below code, I’ve highlighted all the changes I made:

contract NFTMarketWithExpiration {
    struct Listing {
        address seller;
        IERC721 token;
        uint256 tokenId;
        uint256 price;
        uint256 expiration;
    }

    Listing[] public listings;

    function list(
        IERC721 token,
        uint256 tokenId,
        uint256 price,
        uint256 expiration
    )
        external
    {
        listings.push(Listing({
           seller: msg.sender,
           token: token,
           tokenId: tokenId,
           price: price,
           expiration: expiration
        }));
    }

    function buy(uint256 listingId) external payable {
        Listing memory listing = listings[listingId];

        require(listing.seller != address(0), "No such listing.");
        require(now < listing.expiration, "Listing has expired.");
        require(msg.value == listing.price, "Incorrect payment.");

        delete listings[listingId];
        listing.token.transferFrom(
            listing.seller, msg.sender, listing.tokenId);
    }

    function cancel(uint256 listingId) external {
        require(msg.sender == listings[listingId].seller,
            "Only seller can cancel.");

        delete listings[listingId];
    }
}

This time, it took only four lines of code to add the needed functionality! The result is much easier to read and was trivial to get right on the first try. When I add new functions in the future, it will be straightforward to take expirations into account.

Should you use inheritance at all?

Vyper, an Ethereum smart contract language that aims to fix some of Solidity’s shortcomings, does away with inheritance altogether:

Class inheritance — requires people to jump between multiple files to understand what a program is doing, and requires people to understand the rules of precedence in case of conflicts (which class’ function X is the one that’s actually used?). Hence, it makes the code too complicated to understand.

I’d love to tell everyone to just never use inheritance, but here are a few arguments in favor of it:

1. Using inheritance in conjunction with private functions can actually simplify things. A derived class can’t call its parents’ private functions directly, so there’s a little less surface area to consider.
2. Inheritance may make it easier to maintain multiple versions of a contract that support different use cases. OpenZeppelin is an example of a library that uses inheritance in this way. Arguably, inheritance makes these contracts harder to read, but it makes it easier for the developers to maintain lots of variants.
3. If you want to reuse someone else’s code, inheritance may be the cleanest way to do that. Deriving from a contract, especially a popular one like OpenZeppelin’s Ownable contract, may actually make it easier for a reader to understand your code. This is especially true if you don’t override any of the base contract’s functions. Copy/pasting would make it harder for readers to determine whether you made any modifications.

Bottom line

I encourage you to think of inheritance like inline assembly: there are situations that call for its use, but it should be avoided when possible because it hurts readability and maintainability.

All posts