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

Incorrect Removal of Beneficiaries from Array

jufel

Description: In InheritanceManager::removeBeneficiary using the 'delete' keyword sets the element to the zero address rather than removing it from the array. This leaves a "hole" in the array.

Impact: This bug interferes with subsequent logic in functions like InheritanceManager::buyOutEstateNFT and InheritanceManager::withdrawInheritedFunds , potentially causing incorrect calculations and distribution errors. For instance, the zero address may be inadvertently included in the beneficiary count, resulting in misallocated funds.

Proof of Concept: Include the following tests in the InheritanceManagerTest.t.sol file:

function testInheritMultipleBeneficiariesNotOne() public {
address user2 = makeAddr("user2");
​
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.removeBeneficiary(user2);
​
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
​
vm.startPrank(user1);
im.inherit();
vm.stopPrank();
assertEq(true, im.getIsInherited());
}
​
function testWithdrawInheritedFundsAfterDeleteBeneficiary() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.removeBeneficiary(user3);
​
vm.warp(1);
​
uint256 value = 3e18;
vm.deal(address(im), value);
​
vm.warp(1 + 90 days);
​
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
​
uint256 beneficiariesAmount = 2;
uint256 sharePerUser = value / beneficiariesAmount;
​
assertLt(user1.balance, sharePerUser);
assertLt(user2.balance, sharePerUser);
}
​

Recommended Mitigation: Swap the element to remove with the last element and then use the pop operation to remove it from the array. This approach is gas efficient and avoids leaving a zero address in the array.

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

Support

FAQs

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