Summary
The removeBeneficiary
function in InheritableManager.sol
enables the owner to remove an added beneficiary. But to do so, it uses the delete keyword on the target index. This will cause parts of the beneficiaries
array to be populated with address(0)
, meaning that when the funds get distributed a part of it will be effectively burnt.
Vulnerability Details
The issue occurs here:
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
@> delete beneficiaries[indexToRemove];
}
POC
function testUsingDeleteKeywordIsBad() public {
address zeroIndex = makeAddr("zeroIndex");
address firstIndex = makeAddr("firstIndex");
address secondIndex = makeAddr("secondIndex");
vm.startPrank(owner);
im.addBeneficiery(zeroIndex);
im.addBeneficiery(firstIndex);
im.addBeneficiery(secondIndex);
assertEq(firstIndex, im.beneficiaries(1));
im.removeBeneficiary(firstIndex);
vm.stopPrank();
address newFirstindex = im.beneficiaries(1);
assertEq(newFirstindex, address(0));
}
Impact
Beneficiaries will lose their funds
Tools Used
manual review, foundry test suite
Recommendations
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
+ beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
+ beneficiaries.pop();
}