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

Inadequate checks in `InheritanceManager::inherit` allowing anyone to call the function after deadline has passed.

kelbelss

Description

In the inherit function, no checks are done to see if the address/addresses calling the function are even beneficiaries.

Impact

Anyone can call the function and either become the owner or switch isInherited to true, leading to further problems.

Proof of Concept

The test below does the following:

  1. Owner is pranked and they add user1 as a beneficiary.

  2. 90 days later, user2 calls inherit and becomes the owner as proven in the assertEq.

Add the following code to InheritanceManagerTest.t.sol:

function test_inherit_AnyoneCanClaim() public {
address user2 = makeAddr("user2");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
​
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
​
vm.startPrank(user2);
im.inherit();
vm.stopPrank();
​
assertEq(user2, im.getOwner());
}

Tools Used

Manual review, Foundry

Recommended Mitigation

A mapping can be added which allows a gas efficient check to be done to ensure caller is a beneficiary.

  1. Add the following mapping to the codebase:

+ mapping(address => bool) public isBeneficiary;

  1. Add the following to addBeneficiary tp update the mapping:

function addBeneficiery(address _beneficiary) external onlyOwner {
beneficiaries.push(_beneficiary);
+ isBeneficiary[_beneficiary] = true;
_setDeadline();
}

  1. Make sure to set isBeneficiary to False in removeBeneficiary.

  2. Add the following code to inherit:

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

  1. Add error NotABeneficiary(); to codebase.

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!