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

Improper array management in `InheritanceManager::removeBeneficiary` can lead to lost funds

stratan

Summary

delete beneficiaries[indexToRemove] zeroes out the element (sets it to address(0)), but it doesn’t shrink the array’s length or shift elements to fill the "hole". removeBeneficiary is referenced by many critical functions that deal with fund transfers/distribution and depend on beneficiaries.length, leading to incorrect calculations, as well as lost funds.

Vulnerability Details

By using delete to zero out an array slot without adjusting the array's length, the contract creates an array with entries containing address(0).

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
@> delete beneficiaries[indexToRemove];
}

This issue is propagated to core functions of the InheritanceManager contract where funds (ETH, ERC20 tokens) are divided and distributed equally among beneficiaries. Taking withdrawInheritedFunds as an example,

function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
// [1]
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
// [2]
(bool success, ) = beneficiary.call{
value: amountPerBeneficiary
}("");
require(success, "something went wrong");
}
} else {
// ...
}
​
// ...
}

it calculates amountPerBeneficiary ([1]) and then sends out the funds to each beneficiary ([2]). Imagine the contract holds 40 ETH with four beneficiaries ([0xbob, 0xalice, 0xalex, 0xsara]) and 0xalice is removed. The array becomes [0xbob, address(0), 0xalex, 0xsara]. withdrawInheritedFunds will still divide the funds by 4 since it's using beneficiaries.length as the divisor. Sending ETH to address(0) will either burn the funds or revert. Either way, the withdrawal process will break.

Proof of Concept

The code below demonstrates the said vulnerability, which results to lost funds. Place test_improperBeneficiaryRemoval in InheritanceManagerTest.t.sol:

function test_improperBeneficiaryRemoval() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address alex = makeAddr("alex");
address sara = makeAddr("sara");
vm.deal(address(im), 40 ether);
vm.warp(1);
​
// add beneficiaries
vm.startPrank(owner);
im.addBeneficiery(bob);
im.addBeneficiery(alice);
im.addBeneficiery(alex);
im.addBeneficiery(sara);
im.removeBeneficiary(alice);
vm.stopPrank();
​
// trigger deadline expiration
vm.warp(1 + 90 days);
vm.startPrank(bob);
​
// inherit + withdraw funds
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(bob.balance, 10 ether);
assertEq(alice.balance, 0);
assertEq(alex.balance, 10 ether);
assertEq(sara.balance, 10 ether);
assertEq(address(im).balance, 0);
}

And run the test:

$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_improperBeneficiaryRemoval
[⠊] Compiling...
[⠔] Compiling 1 files with Solc 0.8.26
[⠒] Solc 0.8.26 finished in 1.41s
Compiler run successful!
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_improperBeneficiaryRemoval() (gas: 308382)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.69ms (1.94ms CPU time)
​
Ran 1 test suite in 145.20ms (7.69ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

A very core assumption/invariant of the contract is broken, causing incorrect fund distribution and/or loss of funds.

Tools Used

  • Manual review

  • Foundry

Recommendations

Consider shifting the array elements and updating its length.

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
for (uint256 i = indexToRemove; i < beneficiaries.length - 1; i++) {
beneficiaries[i] = beneficiaries[i + 1];
}
beneficiaries.pop();
}
Updates

Lead Judging Commences

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

Give us feedback!