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

Ineffective reentrancy protection due to storage slot mismatch

0xWithdrawers

Description

The nonReentrant modifier in the InheritanceManager contract is intended to prevent reentrancy attacks by using transient storage. However, there is a critical mismatch between the storage slots used for locking and checking. The modifier checks for a lock at slot 1 (tload(1)), but sets the lock at slot 0 (tstore(0, 1)), making the reentrancy protection completely ineffective.

modifier nonReentrant() {
assembly {
if tload(1) { revert(0, 0) }
tstore(0, 1)
}
_;
assembly {
tstore(0, 0)
}
}

This implementation error means that functions with this modifier are vulnerable to reentrancy attacks, despite appearing to be protected.

Impact

  1. Reentrancy vulnerability: Functions protected by this modifier (including sendERC20, sendETH, and contractInteractions) are vulnerable to reentrancy attacks, potentially allowing attackers to drain funds from the contract.

Proof of Concept (PoC)

The following code demonstrates that the nonReentrant modifier does not prevent reentrancy as intended:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../src/InheritanceManager.sol";
​
// This PoC demonstrates that reentrance protection is not effective.
// The code checks for transient storage on slot 1, whereas it stores the lock on slot 0.
​
// This contract extends InheritanceManager to expose the nonReentrant modifier for direct testing
contract NonReentrantTester is InheritanceManager {
bool public firstFunctionCalled = false;
bool public secondFunctionCalled = false;
// Function using the nonReentrant modifier from InheritanceManager
function firstProtectedFunction() external nonReentrant {
firstFunctionCalled = true;
// Call another protected function from within this one
// This should fail if the nonReentrant modifier is working correctly
this.secondProtectedFunction();
}
// Another function with the same nonReentrant modifier
function secondProtectedFunction() external nonReentrant {
secondFunctionCalled = true;
}
}
​
contract PoCTest is Test {
NonReentrantTester public tester;
function setUp() public {
// Deploy our test contract that inherits from InheritanceManager
tester = new NonReentrantTester();
}
// Test if the nonReentrant modifier actually prevents reentrancy
function test_NonReentrantModifier() public {
console.log("Testing the nonReentrant modifier in InheritanceManager");
// Call the first function on the tester contract
tester.firstProtectedFunction();
// Check if both functions were called
bool firstCalled = tester.firstFunctionCalled();
bool secondCalled = tester.secondFunctionCalled();
console.log("First function called:", firstCalled);
console.log("Second function called:", secondCalled);
// If the nonReentrant modifier works correctly, the second function should not be called
// If it's broken, both functions will be called
assertTrue(firstCalled, "First function should have been called");
assertTrue(secondCalled, "Second function was called despite nonReentrant - THIS PROVES THE VULNERABILITY");
console.log("VULNERABILITY CONFIRMED: The nonReentrant modifier in InheritanceManager is flawed!");
console.log("It checks slot 1 with tload(1) but stores the lock in slot 0 with tstore(0, 1)");
console.log("This mismatch allows reentrant calls that should be prevented by the modifier");
}
}

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

forge test --match-test test_NonReentrantModifier -vv

The PoC confirms that both protected functions can be called in sequence, which should not be possible with a properly functioning reentrancy guard.

Recommendation

Fix the mismatch between the storage slots used in the nonReentrant modifier:

modifier nonReentrant() {
assembly {
if tload(0) { revert(0, 0) } // Check the same slot that we lock
tstore(0, 1)
}
_;
assembly {
tstore(0, 0)
}
}

Alternatively, for better clarity and safety, consider using OpenZeppelin's ReentrancyGuard implementation which is well-tested and audited:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract InheritanceManager is Trustee, ReentrancyGuard {
// ...
function sendERC20(...) external nonReentrant onlyOwner {
// ...
}
}

Concrete Impact Example

Consider a scenario where an attacker exploits this vulnerability:

  1. The attacker creates a malicious contract with a fallback function that calls back into the InheritanceManager.sendETH() function

  2. The attacker calls sendETH() with their malicious contract as the recipient

  3. When the InheritanceManager sends ETH to the attacker's contract, the fallback function executes

  4. The fallback function calls back into sendETH(), which should be prevented by the nonReentrant modifier but isn't

  5. This cycle repeats until the contract is drained of ETH

In a real inheritance management system, this could result in one malicious beneficiary stealing all funds from the contract before other legitimate beneficiaries can claim their share, completely undermining the intended inheritance distribution process.

Updates

Lead Judging Commences

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

Wrong value in nonReentrant modifier

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

Wrong value in nonReentrant modifier

Support

FAQs

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