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

Wrong address will delete the first beneficiary

gautr

Summary

It is expected to delete only valid beneficiaries. However, if the owner provides an incorrect address or makes a duplicate call with the same address, the first beneficiary will be deleted.

Vulnerability Details

The InheritanceManager::removeBeneficiary function utilizes the helper function InheritanceManager::_getBeneficiaryIndex, which receives an address and returns its corresponding index in the beneficiaries array. However, if no item matches the provided address, no value will be assigned to _index, so the default value of uint256 (0) will be returned. Since index 0 corresponds to the first beneficiary, this results in the unintended deletion of the first beneficiary.

function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
break;
}
}
}
​
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary); // return 0
delete beneficiaries[indexToRemove]; // first beneficiary removed
}

POC

Add this new getter function on InheritanceManager.sol

function getBeneficiaryAddress(uint256 index) public view returns (address) {
return beneficiaries[index];
}

Add the following test on InheritanceManagerTest.t.sol

function test_wrong_removal_first_beneficiary() public {
address user2 = makeAddr("user2");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
​
im.removeBeneficiary(makeAddr("wrong_user_2"));
vm.stopPrank();
​
assertNotEq(im.getBeneficiaryAddress(0), user1);
assertEq(im.getBeneficiaryAddress(1), user2);
}

Impact

The first beneficiary will be deleted, losing their inheritance rights.

Tools Used

  • Foundry

Recommendations

It is highly recommended to use a mapping(address => bool) to store the beneficiaries, as it is more performant and gas-efficient:

mapping(address => bool) public s_beneficiaries;
​
function removeBeneficiary(address _beneficiary) public {
if(!s_beneficiaries[_beneficiary]) {
revert InvalidBeneficiaries();
}
​
delete s_beneficiaries[_beneficiary];
}

However, if using a for loop to find the beneficiary's index, you should verify whether index 0 corresponds to the intended address. If not, the function should revert:

function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
break;
}
}
​
+ if (_index == 0 && beneficiaries[0] != _beneficiary) {
+ revert InvalidBeneficiaries();
+ }
}
Updates

Lead Judging Commences

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