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

Risk of Overwriting Beneficiary Address at Index Zero, Leading to Potential Loss of Funds

ssalaudeen10

Summary

The beneficiary address at index zero is at risk of being mistakenly removed. This occurs because querying for an address that is not a beneficiary from the list of beneficiaries always returns index zero by default. This creates a significant vulnerability, as an invalid query could inadvertently remove the beneficiary located at index zero, regardless of intent.

Vulnerability Details

The following test case demonstrates the issue and can be added to Inheritance.t.sol:

​
function test_can_remove_user_at_index_zero()public {
// Create an address that is not a beneficiary
address invalidAddress = makeAddr("invalidAddress");
vm.startPrank(owner);
// add user1 as a beneficiary
im.addBeneficiery(user1);
address beneficiaryAtIndex = im.getBeneficiaries()[0];
//asserting that user1 is the beneficiary at index 0
vm.assertEq(beneficiaryAtIndex, user1);
// trying to remove the non-beneficiary address
im.removeBeneficiary(invalidAddress);
beneficiaryAtIndex = im.getBeneficiaries()[0];
// asserting that trying to remove the non-beneficiary address removes the beneficiary at index 0
vm.assertEq(beneficiaryAtIndex, address(0));
vm.stopPrank();
}

Impact

The beneficiary at index zero can be removed unintentionally, potentially leading to loss of funds and breaking the integrity of the

Tools Used

Foundry test

Recommendations

Implement safeguards to ensure that only valid beneficiaries can be removed. The following code demonstrates the fix:

contract InheritanceManager is Trustee {
+ error AddressIsNotBeneficiary();
​
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
//index would be override if the _beneficiary address is gotten
+ _index = type(uint256).max;
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);
+ if(indexToRemove == type(uint256).max){
+ revert AddressIsNotBeneficiary();
+ }
delete beneficiaries[indexToRemove];
​
​
}
}
Updates

Lead Judging Commences

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

Give us feedback!