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

Any non-beneficiary user can become owner and drain the contract funds

vincent71399

Description: The inherit function in the InheritanceManager contract can be invoked by any non-beneficiary user if there is exactly one beneficiary,
resulting in the caller becoming the owner. Then the user takes full control of the contract and can drain the contract of funds.

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();
}
}

Impact: This results in the contract being owned by an unintended user.

Proof of Concept: add the following test case in the InheritanceManager.t.sol file and run the test.

address beneficiary1 = makeAddr("beneficiary1");
...
function test_anyoneCanBecomeOwner(address someUser) public {
assert(someUser != owner && someUser != beneficiary1);
vm.startPrank(owner);
im.addBeneficiery(beneficiary1); // ensure only one beneficiary
vm.stopPrank();
vm.warp(im.getDeadline());
vm.prank(someUser);
im.inherit();
assertEq(im.getOwner(), someUser);
}

Recommended Mitigation:
should limit caller to beneficiaries, if only one benficiary exist, should set owner to the beneficiary.

```solidity
modifier onlyBeneficiary() {
uint256 i = 0;
// @audit-medium beneficiaries.length + 1 will it cause array out-of-bounds, if msg.sender is not in the array or not inherited
while (i < beneficiaries.length) {
if (msg.sender == beneficiaries[i]) {
break;
}
i++;
}
_;
}
...
- function inherit() external {
+ function inherit() external onlyBeneficiary {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
- owner = msg.sender;
+ owner = beneficiaries[0];
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}
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.