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

Deleting an address from an address array, replaces it with address(0), leading to beneficiaries getting less funds (ETH or other assets)

mitrev

Summary

When you use the delete keyword in the context of an array, deleting the variable from the list just initiates it to its default value, which in the context of beneficiaries[] array will be the address(0). Deleting an element like this from an array, doesn't make the array shorter and if there's a beneficiary that has to be removed from the array, all the other beneficiaries that remain in the beneficiaries[] array will not get the right amount of funds (in fact they would get less funds). Some of the funds will get burnt and sent to address(0), when a beneficiary calls InheritanceManager::withdrawInheritedFunds.

Vulnerability Details

Place the following snippet in your test suite, in InheritanceManagerTest.t.sol.

function test_wrongRemoveBeneficiary() public {
//having 3 beneficiaries
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
//Let's have some ether in the contract
vm.deal(address(im), 10e18);
vm.startPrank(owner);
//adding the beneficiaries to the beneficiaries list
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
//user2 is on index 1
assertEq(1, im._getBeneficiaryIndex(user2));
im.removeBeneficiary(user2);
//user2 is removed, but address(0) is on index 1
assertEq(1, im._getBeneficiaryIndex(address(0)));
//the length of beneficiaries array hasn't changed
vm.stopPrank();
//lets assume we disperse ETH here
uint256 beneficiariesLength = 3;
uint256 balance = address(im).balance;
uint256 amountPerBeneficiary = balance / beneficiariesLength;
console.log(amountPerBeneficiary);
//but if we remove the beneficiary from the beneficiaries list the correct way,
//the length of beneficiaries array will change
uint256 correctAmount = balance / 2;
console.log(correctAmount);
assertGt(correctAmount, amountPerBeneficiary);
}

As you can see, the beneficiaries that are left in the array are 2, but the array length is 3. So 1/3 of the funds will be burnt.

Impact

Some of the funds that should be inherited in the future, might be accidentally burnt.

Tools Used

Manual Review

Recommendations

There are 2 ways of properly deleting an element from an array.

Option 1: Shift all elements to remain in the array, that are placed after the one to be removed, to the left. Then pop the last element. This option keeps the correct order of the elements in the array.

Option 2: Replace the element to be removed with the last element of the array and then pop the last elements (as it would be doubled). This option doesn't keep the same order of the elements in the array.

Option 2 is the recommended choice here, because the order of the elements in the beneficiaries[] array doesn't matter and it is a lot more gas efficient to use this method.

In InheritanceManager::removeBeneficiary :

-delete beneficiaries[indexToRemove];
+beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
+beneficiaries.pop();
Updates

Lead Judging Commences

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