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

Improper Beneficiary Removal In InheritanceManager::removeBeneficiary Can Lead To Gap In Array Resulting In Unequal Distribution Of Funds

0xgutzzz

Summary: InheritanceManager::removeBeneficiary improperly deletes an element from the beneficiaries array, leaving a gap which can lead to unexpected behavior in functions requiring sequential indexing such as unequal funds distribution in InheritanceManager::withdrawInheritedFunds.

Description: InheritanceManager::removeBeneficiary deletes an element from the beneficiaries array without shifting remaining elements in the array, leaving an empty slot. This causes incorrect indexing.

Impact: Any function iterating over the beneficiaries array may fail due to unintended empty slots or behave not as intended. Unequal funds distribution in InheritanceManager::withdrawInheritedFunds.

Vulnerability Details: InheritanceManager::removeBeneficiary deletes an element from the beneficiaries array without shifting remaining elements in the array, leaving an empty slot. This causes incorrect indexing, which can lead to unequal distribution of funds in InheritanceManager::withdrawInheritedFunds.

POC: The vulnerable code:

//@audit --> Improper array deletion, can lead to gap in array
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
​

The following test was written, which returned false, proving that other elements in the array were not shifted after one element had been deleted:

function testRemoveBeneficiaryLeavesGap() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
vm.startPrank(owner);
im.removeBeneficiary(user2);
vm.stopPrank();
assertEq(0, im._getBeneficiaryIndex(user1));
assertEq(1, im._getBeneficiaryIndex(user3));
}

Recommendations: Replace delete beneficiaries[indexToRemove]; with a swap-and-pop method, which shifts subsequent elements in the array after one element has been deleted:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
uint256 lastIndex = beneficiaries.length - 1;
​
if (indexToRemove != lastIndex) {
beneficiaries[indexToRemove] = beneficiaries[lastIndex]; // Move last element to deleted slot
}
beneficiaries.pop(); // Remove last element to maintain array integrity
}
Updates

Lead Judging Commences

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

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

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