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

[M-3] Missing Return Check in _getBeneficiaryIndex Can Lead to Removing Wrong Beneficiary

d33p

Summary

The _getBeneficiaryIndex helper function in the InheritanceManager contract returns the default value 0 when a beneficiary isn't found, without any indication of failure. This could lead to removing the wrong beneficiary if the requested address isn't in the array, particularly if there is a valid beneficiary at index 0.

Vulnerability Details

The _getBeneficiaryIndex function is implemented with a loop that returns the index when a match is found, but has no mechanism to indicate when a beneficiary isn't found:

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

This implementation has several issues:

  1. It returns 0 (the default uint256 value) when a beneficiary isn't found

  2. If there is a valid beneficiary at index 0, the caller has no way to distinguish between:

    • The beneficiary was found at index 0

    • The beneficiary wasn't found at all

  3. Functions like removeBeneficiary that use this helper could remove the wrong beneficiary

Impact

This vulnerability could lead to:

  1. Removing the wrong beneficiary (the one at index 0) when attempting to remove a non-existent beneficiary

  2. Incorrect validation of beneficiary status

  3. Potential manipulation of the beneficiary list

The severity is medium because while it could lead to removing the wrong beneficiary, it requires specific conditions (attempting to remove a non-existent beneficiary when there is a valid one at index 0) and the impact is limited to beneficiary management rather than direct fund loss.

Tools Used

Manual code review

Recommendations

Modify the function to clearly indicate when a beneficiary isn't found:

function _getBeneficiaryIndex(
address _beneficiary
) public view returns (uint256 _index, bool _found) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
_found = true;
break;
}
}
}

And update the calling functions to check the return value:

function removeBeneficiary(address _beneficiary) external onlyOwner {
(uint256 indexToRemove, bool found) = _getBeneficiaryIndex(_beneficiary);
require(found, "Beneficiary not found");
delete beneficiaries[indexToRemove];
_setDeadline();
}
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.