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

The onlyBeneficiaryWithIsInherited modifier has broken access control that allows unauthorized access

0xWithdrawers

Description

The onlyBeneficiaryWithIsInherited modifier in the InheritanceManager contract does not implement a proper access control. Instead of explicitly checking permissions and reverting when unauthorized, it relies on an array out-of-bounds error to halt execution for unauthorized callers, and has an additional vulnerability that allows complete bypass under certain conditions.

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}

This modifier has two issues:

  • It relies on an array out-of-bounds error to prevent unauthorized access, rather than explicitly checking permissions and properly revert the transaction in case of failure.

  • It can be completely bypassed by transactions originating from address(0) when there are empty slots in the beneficiaries array. This can occur for example when a contract owner deletes a beneficiary, leaving the slot empty.

Impact

  1. Unreliable access control: The modifier relies on an implicit array out-of-bounds error to prevent unauthorized access rather than explicit checks. This is not a proper security practice and may behave unpredictably across different EVM implementations.

  2. Modifier Bypass: If a beneficiary is removed using removeBeneficiary(), it leaves address(0) in the array. Any transaction from address(0) can then match this entry and bypass access control, allowing attacker to call protected functions.

Proof of Concept (PoC)

The following code demonstrates both vulnerabilities in the modifier:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../src/InheritanceManager.sol";
​
/**
* This test demonstrates the security issues of the onlyBeneficiaryWithIsInherited modifier
*/
contract PocTest is Test {
InheritanceManager target;
address owner = address(0x1);
address beneficiary1 = address(0x2);
address beneficiary2 = address(0x3);
address attacker = address(0x4);
function setUp() public {
// Deploy the contract
vm.prank(owner);
target = new InheritanceManager();
}
function testOnlyBeneficiaryOutOfBounds() public {
// Add two beneficiaries beneficiary1 and beneficiary2 to the contract
vm.prank(owner);
target.addBeneficiery(beneficiary1);
vm.prank(owner);
target.addBeneficiery(beneficiary2);
​
// Set isInherited to true using the inherit function as we have two beneficiaries
// on the contract
vm.warp(target.getDeadline() + 1);
vm.prank(beneficiary1);
target.inherit();
​
// Demonstrate that the modifier relies on array out-of-bounds
// exception instead of proper access control to revert
vm.prank(attacker);
vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x32));
target.appointTrustee(attacker);
}
​
function testOnlyBeneficiaryBypass() public {
// Add two beneficiaries beneficiary1 and beneficiary2 to the contract
vm.prank(owner);
target.addBeneficiery(beneficiary1);
vm.prank(owner);
target.addBeneficiery(beneficiary2);
// Contract owner removes the beneficiary1, thus leaving slot0 with address(0)
vm.prank(owner);
target.removeBeneficiary(beneficiary1);
// Trigger inheritance
vm.warp(target.getDeadline() + 1);
vm.prank(beneficiary2);
target.inherit();
// If isInherited is true, the check "msg.sender == beneficiaries[i] && isInherited"
// would be true when msg.sender is address(0) and i=0
if (target.getIsInherited()) {
vm.prank(address(0));
try target.appointTrustee(attacker) {
// address(0) was used to bypass the modifier access control and
// set the attacker as trustee on the contract
assertEq(target.getTrustee(), attacker);
console.log("Contract appointTrustee: ", target.getTrustee());
} catch {
console.log("Modifier bypass failure");
}
}
}
}

Place the test in the test folder and run it with the following command

forge test --match-test testOnlyBeneficiaryOutOfBounds -vv
forge test --match-test testOnlyBeneficiaryBypass -vv

The PoC confirms that the onlyBeneficiaryWithIsInherited modifier suffers from a broken access control vulnerability.

Recommendation

The inherit() function should ensure that only beneficiaries can call this function and that the ownership cannot be taken over by an attacker.

modifier onlyBeneficiaryWithIsInherited() {
require(isInherited, "inheritance not active");
bool isBeneficiary = false;
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i] && beneficiaries[i] != address(0)) {
isBeneficiary = true;
break;
}
}
require(isBeneficiary, "caller is not a beneficiary");
_;
}

The removeBeneficiary should also probably be reviewed to avoid leaving empty slots when removing beneficiaries.

Concrete Impact Example

Consider a scenario where an attacker exploits this vulnerability:

  1. The contract manages cryptocurrency worth 2B$

  2. Owner adds two beneficiaries, then removes one, leaving address(0) at index 0

  3. After 90 days of inactivity, an attacker sets up a contract that can call from address(0) (possible through specific contract creation techniques)

  4. Then, the attacker uses this to appoint himself as trustee, giving them control over asset revaluation

  5. The attacker can manipulate asset values to gain benefits in various buyout scenarios

This scenario highlights the broken access control issue on this modifier.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Other

Appeal created

reptou Submitter
3 months ago
0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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