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?