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 10 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!