Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Attacker can steal inheritance because of logic flaw in `InheritanceManager::inherit()` function

yeahchibyke

Summary

The ::inherit() function serves two purposes (per the README);

  • Enable an owner who lost their keys to reclaim inheritance from beneficiaries slot0

  • Enable claimers to claim funds after 90 days deadline

but there is a flaw in the logic of this function that can be manipulated by an attacker to steal all the inheritance.

Vulnerability Details

The below lines in ::inherit() stipulates that if the beneficiary for the inheritance is just one person, then whoever called the function (i.e. the msg.sender) should automatically become the owner of the inheritance.

function inherit() external {
...
...
if (beneficiaries.length == 1) {
owner = msg.sender;
_setDeadline();
}
...
...
}

However this check fails to account for the fact that the msg.sender could be a non-beneficiary of the inheritance.

Impact

A non-beneficiary can call ::inherit() for an inheritance that has only one beneficiary, become the owner, and then send the inheritance to themselves by calling either of the ::sendETH() or ::sendERC20() functions.

Tools Used

  • Manual Review

  • Foundry

PoS

Imagine this scenario:

  • A father sets up an inheritance of 100 ETH and adds his only son as the sole beneficiary

  • His jealous brother is aware of the logic flaw in the contract and monitors the inheritance to know when the deadline is up

  • Once the deadline is up, the uncle calls the ::inherit() function and automatically becomes the owner of the inheritance

  • He quickly calls the ::sendETH() function and sends the inheritance to himself

  • The father and son are unaware that the inheritance has been stolen, and even they are aware they cannot do anything as calling the ::inherit() function resets the deadline to 90 days

PoC

Add the following test to the InheritanceManagerTest contract:

function test_inheritance_can_be_stolen() public {
vm.deal(address(im), 100e18);
address son = makeAddr("beneficiary");
address uncle = makeAddr("non_beneficiary");
​
vm.startPrank(owner);
im.addBeneficiery(son);
vm.stopPrank();
​
vm.warp(block.timestamp + 90 days);
​
// uncle calls inherit function
vm.prank(uncle);
im.inherit();
​
// nobody else is able to call inherit function until after 90 days by which time funds have been stolen
vm.prank(son);
vm.expectRevert();
im.inherit();
​
assert(uncle == im.getOwner()); // uncle becomes owner
​
vm.prank(uncle);
im.sendETH(100e18, uncle);
​
assert(address(im).balance == 0);
assert(uncle.balance == 100e18);
}

Recommendations

Since the ::inherit() function is meant to be called by only one of two parties;

  • the owner of the inheritance, i.e., whoever set up the inheritance, or

  • the beneficiary(s),

add a check that ensures that whoever calls this function is one of either parties

// Create a mapping of address to bool
+ mapping(address => bool) isBeneficiary
​
// Implement this mapping in the `addBeneficiary()` function
function addBeneficiery(address _beneficiary) external onlyOwner {
+ require(!isBeneficiary[_beneficiary], "Already a beneficiary!!!");
beneficiaries.push(_beneficiary);
_setDeadline();
+ isBeneficiary[_beneficiary] = true;
}
​
// Then add the following check in the `inherit()` function
function inherit() external {
+ require(isBeneficiary[msg.sender] || msg.sender == owner, "You cannot inherit!!!");
...
...
}

This way, the ::inherit() function is restricted from non-beneficiaries.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Inherit depends on msg.sender so anyone can claim the contract

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.