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

InheritanceManager::inherit() does not have authentication checks and can transfer ownership to msg.sender under some conditions.

vanshikag

Summary

InheritanceManager::inherit() does not have any authentication checks. If there's an item in the beneficiaries array and if the inheritance deadline has passed, anyone can call the function and become the owner of the contract. According to natspec, this functionality exists for account recovery in case the owner loses access to the contract. This is based on the beneficiaries.length == 1 check. Presumably, the owner will have set their address as beneficiaries[0] at some point to allow for this functionality. Even so, the function should set beneficiaries[0] as the owner, and not msg.sender. An attacker could call this function if the current owner forgets to, or they can try to frontrun the owner's transaction.

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

Impact

Anyone can take ownership of the contract by calling the inherit function under certain circumstances.

Proof Of Concept

Copy the following into InheritanceManager.t.sol and run the test.

function test_noAuthCheckOnInherit() public {
address alice = makeAddr("alice");
vm.prank(owner);
im.addBeneficiery(owner);
vm.warp(1 + 90 days);
vm.prank(alice);
im.inherit();
assertEq(alice, im.getOwner());
}

Expected result:

// forge test --mt test_noAuthCheckOnInherit -vvv
[⠢] Compiling...
[⠢] Compiling 1 files with Solc 0.8.26
[⠆] Solc 0.8.26 finished in 4.00s
Compiler run successful!
​
Ran 1 test for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_noAuthCheckOnInherit() (gas: 87955)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 21.19ms (3.00ms CPU time)
​
Ran 1 test suite in 653.87ms (21.19ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommendations

Add authentication check on inherit(). Owner can add a backup address to a list of whitelisted address for account recovery. Additionally, instead of transferring ownership to the msg.sender, the function should transfer ownership to either a beneficiary, or a whitelisted address.

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.