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

If removeBeneficiary is called, ETH is permanently lost and ERC20 tokens are locked in contract

nypet

Summary

The removeBeneficiary function is intended to remove a previously added beneficiary from the beneficiaries array. Instead it just deletes the item within the array (set it's value to 0).

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

If a beneficiary is removed withdrawInheritedFunds will still use the original array size to divide the inheritance. In case of ETH some ETH will be transfered to address(0), so it will be permanently lost. In case of ERC20 tokens the contract will try to transfer some token to he zero address, which will fail, and withdrawInheritedFunds will revert.

Vulnerability Details

The following test demonstrates that out of an initial 12 ETH 4 ETH is permanently lost if one of three beneficiaries is removed.

function testContractSendETHhWellAfterRemovingBeneficiary() public {
vm.deal(address(im), 12 ether);
vm.startPrank(owner);
im.addBeneficiery(aladar);
im.addBeneficiery(bloki);
im.addBeneficiery(kriszta);
im.removeBeneficiary(bloki);
vm.stopPrank();
vm.warp(1 + 90 days);
vm.startPrank(aladar);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
//12 ether should be divided int two equal halves, aladar should get 6
assertEq(aladar.balance, 6 ether);
}

This test demonstrates, that withdrawInheritedFunds reverts if it is called with an ERC20 address after beneficiary was removed.

function testContractSendsWETHWellAfterRemovingBeneficiary() public {
weth.mint(address(im), 12000);
vm.startPrank(owner);
im.addBeneficiery(aladar);
im.addBeneficiery(bloki);
im.addBeneficiery(kriszta);
im.removeBeneficiary(bloki);
vm.stopPrank();
vm.warp(1 + 90 days);
vm.startPrank(aladar);
im.inherit();
im.withdrawInheritedFunds(address(weth));
vm.stopPrank();
//12000 ether should be divided int two equal halves, so aladar should get 6000
assertEq(weth.balanceOf(aladar), 6000);
}

Impact

ETH is permanently lost, or ERC20 toke is permanently locked into the contract if removeBeneficiary is ever called by the owner.

Tools Used

Manual review and foundry test.

Recommendations

Remove the beneficiary from the array. E.g. set the last value of the array to the one to be removed, and pop the last value from the array.

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.