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

Improper beneficiary removal logic leads to loss of funds

olayinkablexxy

Summary

address(0) causes transfer of ether and ERC20 tokens to be stuck in contract forever.

Vulnerability Details

The InheritanceManager::removeBeneficiary function deletes a beneficiary by setting the address at the specified index to address(0). However, this approach does not reduce the length of the beneficiaries array. As a result, the beneficiaries array may contain address(0) entries, which are not valid addresses. When the InheritanceManager::withdrawInheritedFunds function attempts to distribute funds to beneficiaries, it will iterate over the entire array, including address(0) entries. This will cause the function to revert when it tries to transfer funds to address(0), effectively locking the funds in the contract.

POC

Place the test below in ./test/InheritanceManagerTest.t.sol file:

function test_zero_address_leads_to_loss_of_funds() external {
uint TOTAL_FUNDS = 10e18;
weth.mint(address(im), TOTAL_FUNDS);
​
address user2 = makeAddr("user2");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
vm.stopPrank();
​
assertEq(0, im._getBeneficiaryIndex(user1));
assertEq(1, im._getBeneficiaryIndex(user2));
​
vm.startPrank(owner);
im.removeBeneficiary(user2);
vm.stopPrank();
​
// beneficiaries contains address(0) at this point
​
vm.warp(im.getDeadline() + 90 days + 1 seconds);
​
vm.prank(user1);
im.inherit();
​
// Poor user experience. Owner thinks user1 is now a backup because they believe they have only one address as the beneficiary
​
vm.prank(user1);
vm.expectRevert(); // withdraw fails
im.withdrawInheritedFunds(address(weth));
​
assertNotEq(TOTAL_FUNDS, weth.balanceOf(user1));
}

Impact

Loss of funds and poor user experience

Tools Used

  • Foundry

  • Manual Review

Recommendations

To properly remove a beneficiary and avoid leaving address(0) entries in the beneficiaries array, update the Inheritance::removeBeneficiary function to shift the array elements and reduce its length. Two methods can be used: Array only, or array and mappings.

Array Only

The InheritanceManager::removeBeneficiary function to shift the array elements and reduce its length. This corrects the user experience and also removes address(0).

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
+ require(indexToRemove < beneficiaries.length, "Beneficiary not found");
​
+ // Shift elements to the left
+ for (uint256 i = indexToRemove; i < beneficiaries.length - 1; i++) {
+ beneficiaries[i] = beneficiaries[i + 1];
+ }
​
+ // Reduce the array length
+ beneficiaries.pop();
+ _setDeadline(); // Reset the inactivity period
}

Array and Mapping

This method has an advantage of making sure duplicate addresses are not added.

+ mapping(address => bool) public isBeneficiary;
address[] public beneficiaries;

onlyBeneficiaryWithIsInherited Modifier

modifier onlyBeneficiaryWithIsInherited() {
+ require(isBeneficiary[msg.sender] && isInherited);
​
- uint256 i = 0;
- while (i < beneficiaries.length + 1) {
- if (msg.sender == beneficiaries[i] && isInherited) {
- break;
- }
- i++;
- }
_;
}

Add beneficiary

function addBeneficiary(address _beneficiary) external onlyOwner {
+ require(!isBeneficiary[_beneficiary], "Address is already a beneficiary");
+ isBeneficiary[_beneficiary] = true;
beneficiaries.push(_beneficiary);
_setDeadline();
}

Remove beneficiary

function removeBeneficiary(address _beneficiary) external onlyOwner {
- uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
+ require(isBeneficiary[_beneficiary], "Address is not a beneficiary");
​
+ isBeneficiary[_beneficiary] = false;
​
+ for (uint256 i = 0; i < beneficiaries.length; i++) {
+ if (beneficiaries[i] == _beneficiary) {
+ // Swap with the last element and pop
+ beneficiaries[i] = beneficiaries[beneficiaries.length - 1];
+ beneficiaries.pop();
+ break;
+ }
+ }
​
+ _setDeadline();
}
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.