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

[H-1] Anyone can inherit the InheritanceManager contract, should only be the benificiaries and the logic for inheritance is also flawed

yashsharma22003

Summary

This is my first solo audit of any sort, unfortunately due to me coming across this contest just before a few hours of its completion I couldn't find much of the vunerabilities. Sorry if you find it difiicult to read my report, I will definitely improve.

Vulnerability Details

[H-1] Anyone can inherit the InheritanceManager contract, should only be the benificaries

Impact

[H-1] Severly breaks the protocol, as anyone can call the function InheritanceManager::inherit after deadline which will transfer the ownership to any msg.sender who is not even a benificary, also the logic for inheritance is also wrong.

Affected code

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

Tools Used

Tools used - Foundry

Proof of code

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

This test will pass and transfer the ownership to unknown actor.

Recommendations

[H-1]

Mitigation

Add InheritanceManager::onlyBeneficiary modifier in the contract to ensure only benificiary can call the function. (here we have corrected the logic inside the inherit function which was also wrong)

modifier onlyBeneficiary() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i]) {
break;
}
i++;
}
_;
}
function inherit() external onlyBeneficiary {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length >= 1) {
owner = beneficiaries[0];
isInherited = true;
deadline = 0;
}
}
Updates

Lead Judging Commences

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