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

Incorrect Logic in `InheritanceManager::inherit` function with Single Beneficiary Allows Attacker to Claim Ownership of the Contract

roccomania

Summary

The inherit function in the InheritanceManager contract, designed to transfer control of the contract to the beneficiaries after a period of owner inactivity, has a critical flaw in its handling of cases with only a single beneficiary. In this scenario, any address can claim ownership, not just the intended beneficiary, leading to a complete bypass of the intended inheritance mechanism.

Vulnerability Details

  1. inherit Function:

    • Purpose: To transfer ownership of the contract to the beneficiaries after a specified period of owner inactivity (defined by TIMELOCK).

    • Logic:

      • Checks if the current time is past the deadline.

      • If there's only one beneficiary (beneficiaries.length == 1), it directly assigns ownership to msg.sender.

      • If there are multiple beneficiaries (beneficiaries.length > 1), it sets isInherited to true, allowing beneficiaries to distribute funds.

      • If there are no beneficiaries, it reverts with InvalidBeneficiaries().

    • Flaw: The condition beneficiaries.length == 1 only checks the number of beneficiaries and doesn't verify if msg.sender is that specific beneficiary.

    • Code:

      function inherit() external {
      if (block.timestamp < getDeadline()) {
      revert InactivityPeriodNotLongEnough();
      }
      if (beneficiaries.length == 1) { // @audit-issue anybody can claim owner role
      owner = msg.sender; // Any address can claim ownership!
      _setDeadline();
      } else if (beneficiaries.length > 1) {
      isInherited = true;
      } else {
      revert InvalidBeneficiaries();
      }
      }

Proof of Concept

Add this test to the test suite InheritanceManagerTest at path test/InheritanceManagerTest.t.sol and run the command forge test --mt test_inheritByAttacker:

  • From the test, an attacker was able to call the inheritfunction even though he has not interacted with the contract before or added to beneficiry list

function test_inheritByAttacker() public {
address attacker = makeAddr("attacker");
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
vm.startPrank(attacker); //Attacker calls inherit
im.inherit();
vm.stopPrank();
assertEq(attacker, im.getOwner()); // Assertion that the attacker owns the contract
}

Impact

  1. Loss of Funds: A malicious actor can become the owner and transfer all funds out of the contract.

  2. Unauthorized Control: An unintended party can take control of the contract, locking out the original owner and intended beneficiaries.

Tools Used

  • Manual Code Review

  • Foundry for PoC

Recommendations

  • Check msg.sender Against the Beneficiary: Modify the inherit function to verify that msg.sender is the single beneficiary when beneficiaries.length == 1.

    function inherit() external {
    if (block.timestamp < getDeadline()) {
    revert InactivityPeriodNotLongEnough();
    }
    - if (beneficiaries.length == 1) {
    + if (beneficiaries.length == 1 && msg.sender == beneficiaries[0]) {
    owner = msg.sender;
    _setDeadline();
    } else if (beneficiaries.length > 1) {
    isInherited = true;
    - } else {
    + } else if (beneficiaries.length == 0){
    revert InvalidBeneficiaries();
    }
    + else {
    + revert NotBeneficiary();
    + }
    }
Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Inherit depends on msg.sender so anyone can claim the contract

Support

FAQs

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