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

Silent removal of first beneficiary when non-existent address is passed to `removeBeneficiary`

swarecito

Summary

The removeBeneficiary function silently removes the first beneficiary when attempting to remove an address that doesn't exist in the beneficiaries array. This occurs because the _getBeneficiaryIndex helper function returns 0 (the default value for uint256) when the specified address isn't found, causing the function to delete the beneficiary at index 0 without any error or warning.

Vulnerability Details

The vulnerability exists in the interaction between the removeBeneficiary function and its helper function _getBeneficiaryIndex:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
​
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
break;
}
}
// No explicit return if address not found, so _index defaults to 0
}

When _beneficiary is not found in the array, the function completes without setting _index, which means it retains its default value of 0. The calling function then blindly uses this index to delete an entry, removing the first beneficiary rather than the intended one.

Additionally, there are two further issues in this function:

  1. It uses delete on an array element which sets it to the default value (address(0)) but doesn't remove the element from the array

  2. It doesn't call _setDeadline() to update the inactivity timer

Impact

This vulnerability has serious consequences for the inheritance management:

  1. Unintended removal of legitimate beneficiaries

  2. Loss of inheritance rights for the first beneficiary without any notification

  3. If owner is the only beneficiary (in backup wallet scenario), could result in complete loss of fund access

  4. No error message or event to indicate the failure

  5. The contract state becomes inconsistent with the owner's intentions

The impact is rated as medium because:

  • It doesn't directly lead to immediate loss of funds

  • But it could result in fund distribution to wrong parties during inheritance

  • It fundamentally breaks a key contract mechanism

Tools Used

Manual code review and Foundry testing

Proof of Concept

  1. Add the following test to InheritanceManager.t.sol

function test_removeFirstBeneficiaryByPassingInexistentIndex() public {
address user2 = makeAddr("user2");
address inexistentUser = makeAddr("inexistentUser");
​
vm.startPrank(owner);
​
im.addBeneficiery(user1);
im.addBeneficiery(user2);
​
assertEq(user1, im.getBeneficiary(0)); // user1 is the first beneficiary
assertEq(user2, im.getBeneficiary(1));
​
im.removeBeneficiary(inexistentUser); // inexistentUser is not in the beneficiaries array
vm.stopPrank();
​
assert(user1 != im.getBeneficiary(0)); // user1 is not the first beneficiary anymore
}

  1. Run the test

forge test --mt test_removeFirstBeneficiaryByPassingInexistentIndex

Recommendations

  1. Modify the removeBeneficiary function to check if the address exists in the array:

function removeBeneficiary(address _beneficiary) external onlyOwner {
bool found = false;
uint256 indexToRemove = 0;
​
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (beneficiaries[i] == _beneficiary) {
indexToRemove = i;
found = true;
break;
}
}
​
require(found, "InheritanceManager: beneficiary not found");
​
// Use swap and pop pattern for efficient removal
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
beneficiaries.pop();
​
// Reset deadline
_setDeadline();
}

  1. Also implement utility functions to make the contract more user-friendly:

// Check if an address is a beneficiary
function isBeneficiary(address _address) public view returns (bool) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (beneficiaries[i] == _address) {
return true;
}
}
return false;
}
​
// Get total number of beneficiaries
function getBeneficiariesCount() public view returns (uint256) {
return beneficiaries.length;
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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