Underhanded Solidity: Exploiting multiple inheritance

Question

In an article by Steve Marx (on Medium.com), the author argues against using inheritance in solidity not only because of harder readability of a Smart Contract, but also because it can be exploited in an underhanded fashion.

To support his claim, he poses the following vulnerable Bank contract:

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);
    }
}

He argues: The expected behaviour is for the deployers to call disableAdmin() once the contract is off its testing period, and thus we expect the kill() function to be inaccessible.

BUT

Due to how contract Bank inherits is TempAdmin, MultiAdmin, kill() can still be ran and will succeed, therefore admins can suicide the contract and run off with everyone's ether.

The author says that this is due to Solidity following C3 Linearization: Because of how inheritance is resolved, the require(isAdmin()); in kill() results in the equivalent check of extraAdmin[msg.sender] || (administratable && msg.sender == admin), which of course is vulnerable and allows the require(isAdmin()) to return true, and kill() continues execution.

I fail to see how he reaches this conlusion, any help understanding if the article author's claim is valid?

How is inheritance resolved when you've got multiple parents with the same function name?

Answer 1

The 'exploit' no longer applies to solc v0.6, since then the compiler generates an error when there's an ambiguous call.

---

As the author says solc v0.5 uses C3 linearization to determine the order in which functions will be called.

In the cited example the C3 linearization order is [Bank, MultiAdmin, TempAdmin, Admin].

When funct() is called the first implementation in the order will be executed. It can call the next in the sequence with super.funct().

Continuing with the example isAdmin resolves to MultiAdmin.isAdmin()

extraAdmins[msg.sender] || super.isAdmin()

Which invokes the next TempAdmin.isAdmin()

administratable && super.isAdmin();

Which calls the next and final Admin.isAdmin()

msg.sender == admin

Putting all of this together we get

(extraAdmins[msg.sender] || (administratable && (msg.sender == admin)))
                                               \_______ Admin ______/
                           \___________ TempAdmin ___________________/
\____________________ MultiAdmin _____________________________________/

---

How can we fix this?

In the cited example an easy and bad fix is to swap MultiAdmin and TempAdmin in the defintion of Bank

contract Bank is MultiAdmin, TempAdmin {

It works for isAdmin but this change will also affect other functions that we haven't considered. How can we prevent someone else to swap them at a later time? Or to add another inheritance that will change the C3 order? Unfortunately there's no guarantee.

I'd prefer to not depend on C3 linearization if there's some ambiguity.

In the example why do three contracts change isAdmin behavior? Isn't it better to concentrate all such properties in one contract instead?