Description
The removeBeneficiary() function has a critical vulnerability where attempting to remove a non-existent beneficiary will result in the deletion of the first beneficiary (at index 0) instead. This occurs because the _getBeneficiaryIndex() function returns 0 for non-existent beneficiaries due to the default initialization of the return value.
The issue exists in the following code:
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;
}
}
}
When _getBeneficiaryIndex() is called with an address that doesn't exist in the beneficiaries array, the function will complete its loop without finding a match. Since the return variable _index is implicitly initialized to 0, the function returns 0 for non-existent beneficiaries.
Impact
This vulnerability has several severe consequences:
Accidental Deletion: The owner could unintentionally delete the first (potentially most important) beneficiary when attempting to remove a non-existent address.
Inheritance Disruption: Since the beneficiaries array is central to the inheritance logic, removing the wrong beneficiary could significantly disrupt the intended distribution of assets.
Unpredictable Behavior: The function behaves contrary to expectations, potentially leading to confusion and administrative errors.
Proof of Concept
function test_removeBeneficiary() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
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._getBeneficiaryIndex(user3);
im.removeBeneficiary(user3);
vm.stopPrank();
im.beneficiaries(0);
}
Output:
[118764] InheritanceManagerTest::test_removeBeneficiary()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec]
├─ [0] VM::label(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec], "user3")
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [944] InheritanceManager::_getBeneficiaryIndex(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [1425] InheritanceManager::_getBeneficiaryIndex(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 1
├─ [0] VM::assertEq(1, 1) [staticcall]
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [1550] InheritanceManager::_getBeneficiaryIndex(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec]) [staticcall]
│ └─ ← [Return] 0
├─ [2020] InheritanceManager::removeBeneficiary(user3: [0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [732] InheritanceManager::beneficiaries(0) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000
└─ ← [Stop]
Recommendation
Implement proper validation to ensure the beneficiary exists before attempting removal. The function should revert if the beneficiary is not found:
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
return i;
}
}
revert("Beneficiary not found");
}
Tools Used
Manual Code Review
Static Analysis
Foundry test
