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

[M-1] InheritanceManager::removeBeneficiary does not update the length of the array

0xnick

Summary

The removeBeneficiary function does not update the length of the array beneficiaries, leaving address(0) in the 'deleted' slot.

Vulnerability Details

If there are a lot of additions and removals of beneficiaries, this can lead to a large size array and a DoS attack. When we call withdrawInheritedFunds this will send funds to address(0) and result in a loss of funds.

Likelihood: High. Easily triggered by normal owner actions (adding/removing beneficiaries), with guaranteed fund loss if withdrawals occur.

function test_removeBeneficiaryDoesNotUpdateLength() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
vm.startPrank(owner);
​
im.addBeneficiery(user2);
im.addBeneficiery(user3);
​
uint256 beneficiariesBefore = im.getBeneficiaryCount();
​
im.removeBeneficiary(makeAddr("user10"));
​
uint256 beneficiariesAfter = im.getBeneficiaryCount();
​
vm.stopPrank();
​
assertEq(beneficiariesBefore, beneficiariesAfter);
}

Impact

High. Leads to array with address(0) in the 'gaps' which will send assets to non-retrievable address.

Tools Used

  • Manual Review

Recommendations

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
​
+ beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
+ beneficiaries[beneficiaries.length - 1] = address(0);
​
+ beneficiaries.pop();
}
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.