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

InheritanceManager::removeBeneficiary() function does not properly remove beneficiary leading to miscalculation in inheritance distribution

yeahchibyke

Summary

The owner can call ::removeBeneficiary() on any of the previously added beneficiary(s), and this beneficiary will be removed from the list of beneficiaries. At least that is what this function is meant to do...

Vulnerability Details

Improper logic in the ::removeBeneficiary function means that a selected beneficiary is not properly removed from the beneficiaries list.

Impact

The InheritanceManager::withdrawInheritedFunds() function will always miscalculate distribution of inheritance as it will keep on taking into consideration the already removed beneficiary.

Tools Used

  • Manual Review

  • Foundry

PoC

Here are a couple of pocs for this finding:

  • Consider this scenario where 2 beneficiaries are added

  • The owner removes one of the beneficiaries

  • The remaining beneficiary calls ::inherit(), but doesn't get appointed as owner despite them being the only remaining beneficiary

  • They go ahead to call ::withdrawInheritedFunds(), but only get half of the inheritance, with the remaining half sent to zero address (in the case of ETH)

function test_distribution_error() public {
address son = makeAddr("wayward_son");
address daughter = makeAddr("good_daughter");
​
deal(address(im), 100e18);
​
vm.startPrank(owner);
im.addBeneficiery(son);
im.addBeneficiery(daughter);
vm.stopPrank();
​
vm.prank(owner);
im.removeBeneficiary(son);
​
vm.warp(1 + 90 days);
​
vm.prank(daughter);
im.inherit();
​
assert(im.getOwner() != daughter); // daughter should be the owner since she is the only beneficiary after son was removed
​
vm.prank(daughter);
im.withdrawInheritedFunds(address(0));
​
assert(daughter.balance == 50e18); // daughter should get the full inheritance, instead she gets half
}
​
// Monitor the traces to see that the remaining 50 ETH is sent to the zero address, thus unwithdrawable
​
[PASS] test_distribution_error() (gas: 186100)
Traces:
[206000] InheritanceManagerTest::test_distribution_error()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] wayward_son: [0x4C8995C378022554e38829a7c0486Cf7b655a037]
├─ [0] VM::label(wayward_son: [0x4C8995C378022554e38829a7c0486Cf7b655a037], "wayward_son")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] good_daughter: [0x7c499a12c8d15a71b329941b8d5816b011c8f861]
├─ [0] VM::label(good_daughter: [0x7c499a12c8d15a71b329941b8d5816b011c8f861], "good_daughter")
│ └─ ← [Return]
├─ [0] VM::deal(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 100000000000000000000 [1e20])
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69404] InheritanceManager::addBeneficiery(wayward_son: [0x4C8995C378022554e38829a7c0486Cf7b655a037])
│ └─ ← [Stop]
├─ [23504] InheritanceManager::addBeneficiery(good_daughter: [0x7c499a12c8d15a71b329941b8d5816b011c8f861])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::prank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [1708] InheritanceManager::removeBeneficiary(wayward_son: [0x4C8995C378022554e38829a7c0486Cf7b655a037])
│ └─ ← [Stop]
├─ [0] VM::warp(7776001 [7.776e6])
│ └─ ← [Return]
├─ [0] VM::prank(good_daughter: [0x7c499a12c8d15a71b329941b8d5816b011c8f861])
│ └─ ← [Return]
├─ [22791] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [604] InheritanceManager::getOwner() [staticcall]
│ └─ ← [Return] owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
├─ [0] VM::prank(good_daughter: [0x7c499a12c8d15a71b329941b8d5816b011c8f861])
│ └─ ← [Return]
├─ [68495] InheritanceManager::withdrawInheritedFunds(0x0000000000000000000000000000000000000000)
│ ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 50000000000000000000}() 👈🏾👈🏾
│ │ └─ ← [Stop]
│ ├─ [0] good_daughter::fallback{value: 50000000000000000000}()
│ │ └─ ← [Stop]
│ └─ ← [Stop]
└─ ← [Stop]

Recommendations

I added a isBeneficiary mapping, and implemented this in both the addBeneficiary() and removeBeneficiary() functions.

This means that removals are properly handled, and distribution of inheritance is calculated properly.

+ mapping(address => bool) isBeneficiary;
​
function addBeneficiery(address _beneficiary) external onlyOwner {
+ require(!isBeneficiary[_beneficiary], "Already a beneficiary!!!");
beneficiaries.push(_beneficiary);
_setDeadline();
+ isBeneficiary[_beneficiary] = true;
}
​
function removeBeneficiary(address _beneficiary) external onlyOwner {
- uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
+ uint256 bl = beneficiaries.length; // bl = beneficiaries_length
+ for (uint256 b = 0; b < bl; b++) {
+ if (beneficiaries[b] == _beneficiary) {
+ beneficiaries[b] = beneficiaries[bl - 1]; // replace with last element
+ beneficiaries.pop(); // remove last element
+ break;
+ }
+ }
+ isBeneficiary[_beneficiary] = false;
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

Support

FAQs

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