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

Array out-of-bounds access error in the `InheritanceManager::onlyBeneficiaryWithIsInherited`modifier and does not prevent non beneficiary call beneficiary restricted function, Any function required only beneficiaries to call it would revert

ssalaudeen10

Summary

The InheritanceManager::onlyBeneficiaryWithIsInherited modifier is intended to restrict access to functions, ensuring that only valid beneficiaries can call the functions and the isInherited flag is true. However, there are several issues with the current implementation

  1. Out-of-Bound Looping: The loop condition i < beneficiaries.length + 1 results in out-of-bound access, which can cause runtime errors.

  2. Gas Inefficiency: The current implementation is gas-expensive due to the loop iterating over the entire array.

  3. Insufficient Validation: There is no proper check to ensure only beneficiaries can call the function, leading to potential security risks.

Vulnerability Details

The follow test case shows how a beneficiary is unable to appoint a trustee

function test_beneficiary_cannot_appoint_trustee ()public{
// Create new trustee address
address trustee = makeAddr("trustee");
vm.prank(owner);
//Owner adds user 1 as beneficiary
im.addBeneficiery(user1);
vm.expectRevert();
// User one trying to appoint trustee would revert due to array out of bound
vm.prank(user1);
vm.expectRevert();
im.appointTrustee(trustee);
​
}

Impact

The issues identified can lead to the following problems:

  1. Potential runtime errors due to out-of-bound access.

  2. Increased gas costs, making the contract less efficient and more expensive to execute.

  3. Lack of proper validation could allow unauthorized users to call restricted functions, compromising the security of the contract

Tools Used

Foundry test

Recommendations

To mitigate this issue, create a mapping that maps addresses to a Boolean value. When an address is added as a beneficiary, the value is set to true, and when it is removed, it is set to false. This mapping can be used in the modifier to verify if an address is a beneficiary. This implementation improves gas efficiency and ensures proper validation. Below is the code implementation:

​
contract InheritanceManager is Trustee {
+ error OnlyBeneficiaryIsAllowed()
+ mapping(address => bool) addressAlreadyBeneficiary;
​
// mapping should be updated when beneficiary is added and removed
​
modifier onlyBeneficiaryWithIsInherited() {
- uint256 i = 0;
- while (i < beneficiaries.length + 1) {
- if (msg.sender == beneficiaries[i] && isInherited) {
- break;
- }
- i++;
- }
- _;
+ if(!addressAlreadyBeneficiary[msg.sender]){
+ revert OnlyBeneficiaryIsAllowed();
+ }
+ if(!isInherited){
+ revert NotYetInherited();
+ }
+ _;
}
}

Key Improvements:

  1. Prevent Out-of-Bound Looping: The loop condition is removed, and a direct mapping lookup is used.

  2. Optimize Gas Usage: The mapping allows for constant-time lookup (O(1)), making it more gas-efficient compared to iterating through the array.

  3. Enhance Validation: The require statements ensure that only valid beneficiaries with the isInherited flag set to true can proceed, adding a strong layer of security.

By implementing these improvements, the contract will be more secure, efficient, and reliable, preventing potential runtime errors and unauthorized access while optimizing gas usage.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!