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

`InheritanceManager:onlyBeneficiaryWithIsInherited` modifier is misconfigured

fels21

Summary

Due to bad implementation, the InheritanceManager:onlyBeneficiaryWithIsInherited does not function as expected and doesn't revert anything.

Vulnerability Details

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}

Explanation:
First of all, beneficiaries.length + 1 is unnecessary because it will access out of bounds because the index starts from 0. Nevertheless, even without +1, it will not work as expected.

Test:

function test_buyWithoutBeBeneficiary() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address outsider = makeAddr("outsider");
​
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.createEstateNFT("our beach-house", 3e6, address(usdc));
vm.stopPrank();
usdc.mint(outsider, 4e6);
​
vm.warp(1 + 90 days);
vm.startPrank(outsider);
​
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
​
console.log("Balances after:");
console.log("User1:", usdc.balanceOf(user1));
console.log("User2:", usdc.balanceOf(user2));
console.log("User3:", usdc.balanceOf(user3));
console.log("Contract:", usdc.balanceOf(address(im)));
}
Output:
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
\[PASS] test\_buyWithoutBeBeneficiary() (gas: 487836)
Logs:
Balances after:
User1: 666666
User2: 666666
User3: 666666
Contract: 2
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.82ms

Explanation:
This happens because break; is not breaking the execution and only breaks the while loop.

Impact

Very high:
All functions with this modifier are accessible to everyone:

  • InheritanceManager:appointTrustee

  • InheritanceManager:buyOutEstateNFT

Tools Used

Manual review, Forge

Recommendations

Reimplement this function using require:

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
+ bool allowed = false;
while (i < beneficiaries.length) {
if (msg.sender == beneficiaries[i] && isInherited) {
+ allowed = true;
}
i++;
}
+ require(allowed, "Not Allowed");
_;
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

fels21 Submitter
3 months ago
0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.