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

`_getBeneficiaryIndex` does not work when querying non-existent beneficiary

skyge

Summary

The _getBeneficiaryIndex function encounters an issue where the beneficiary index retrieval process lacks proper handling for cases where the beneficiary is not found in the beneficiaries array.

Vulnerability Details

The _getBeneficiaryIndex function iterates through the beneficiaries array to find the index of a specific beneficiary. However, it does not handle scenarios where the beneficiary is not found in the array. As a result, if the beneficiary is not present, it will return 0, that is the first element of the beneficiaries array, and the removeBeneficiary function use it to remove a _beneficiary account, so when a non-existent _beneficiary account is specified for removal, it will result in the removal of the first account in the array instead of throwing an error.

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;
}
}
}

Here is the test case:

function test_removeNonBeneficiary() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
vm.startPrank(owner);
im.addBeneficiery(alice);
assertEq(
bytes32(uint256(uint160(address(alice)))),
vm.load(address(im), bytes32(uint256(keccak256(abi.encode(5))) + 0))
);
​
// Remove the beneficiary with bob address
im.removeBeneficiary(bob);
// Alice has been removed mistakenly
assertEq(
bytes32(uint256(uint160(address(0)))),
vm.load(address(im), bytes32(uint256(keccak256(abi.encode(5))) + 0))
);
vm.stopPrank();
}

Impact

The current implementation of the _getBeneficiaryIndex function does not work when attempting to retrieve the index of a beneficiary that does not exist in the beneficiaries array. And do damage to relative functions, such as removeBeneficiary

Tools Used

Manual

Recommendations

Use EnumerableSet of the OpenZeppelin:

// Import the EnumerableSet library from OpenZeppelin
import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
xxx;
​
contract InheritanceManager is Trustee {
using EnumerableSet for EnumerableSet.AddressSet;
​
EnumerableSet.AddressSet private beneficiaries;
​
// Function to remove a beneficiary using EnumerableSet
function removeBeneficiary(address _beneficiary) external onlyOwner {
require(beneficiaries.remove(_beneficiary), "Beneficiary does not exist");
_setDeadline();
}
​
// Function to get the index of a beneficiary in the set
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
require(beneficiaries.contains(_beneficiary), "Beneficiary not found");
for (uint256 i = 0; i < beneficiaries.length(); i++) {
if (beneficiaries.at(i) == _beneficiary) {
_index = i;
break;
}
}
}
}
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.