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

[H-1] An invalid array element deletion in `InheritanceManager::removeBeneficiary` leads to a loss of funds when a beneficiary calls `InheritanceManager::withdrawInheritedFunds`, with ether being burned at address(0)

kagen

Description: An incorrect array element deletion is used to remove a beneficiary in InheritanceManager::removeBeneficiary. When delete is called on an indexed array element in InheritanceManager::beneficiaries, the array element at that index is set to its default. In this case, an address array has its element set to address(0). Also the array length remains the same and is not decremented. The result is instead of deleting a beneficiary, a beneficiary's address is changed to address(0).

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove]; // @audit This doesn't delete the array entry
}

Impact: If the owner calls InheritanceManager::removeBeneficiary to remove a beneficiary, a future call to withdraw ether withInheritanceManager::withdrawInheritedFunds leads to ether being sent to that "removed" beneficiary at address(0). That ether is burned and lost.

Proof of Concept:

Add the following unit test to InheritanceManagerTest.t.sol:

function test_removeBeneficiaryLossOfFunds() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
// Add user1, user2, and user3 as beneficiaries
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
​
// Deposit 200 ether into InheritanceManager contract
vm.warp(1);
vm.deal(address(im), 200e18);
​
// Skip ahead 90 days so timelock is expired
vm.warp(1 + 90 days);
​
// Remove user3 as beneficiary
vm.prank(owner);
im.removeBeneficiary(user3);
​
// User1 and User2 inherit the funds
vm.startPrank(user1);
im.inherit();
​
// Withdraw ether from InheritanceManager contract
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
​
// User1 and User2 do not have 100 ether each
console.log("user1.balance (ETH)", user1.balance);
console.log("user2.balance (ETH)", user2.balance);
assertNotEq(user1.balance, 100e18);
assertNotEq(user2.balance, 100e18);
}

Run the unit test with verbosity: forge test --mt test_removeBeneficiaryLossOfFunds -vvvv

Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_removeBeneficiaryLossOfFunds() (gas: 257488)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.93ms (1.37ms CPU time)

Further verification is in the stack trace with the 3rd beneficiary having ether sent to address 0x0000000000000000000000000000000000000000 :

│ ├─ [0] user1::fallback{value: 66666666666666666666}()
│ │ └─ ← [Stop]
│ ├─ [0] user2::fallback{value: 66666666666666666666}()
│ │ └─ ← [Stop]
│ ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 66666666666666666666}()

Recommended Mitigation:

The proper way to delete an array element is with the "swap and pop":

  1. Take the last element in the array and copy to the position of the element to be removed.

  2. Remove the last element with with a pop(). This also decreases the array length by 1.

Add the "swap and pop" to InheritanceManager::removeBeneficiary for proper deletion:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
// Move the last element to the position of the element to delete
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
// Remove the last element
beneficiaries.pop();
}
Updates

Lead Judging Commences

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

Give us feedback!