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

Broken nonReentrant Modifier: Attackers Can Recursively Drain Funds

teoslaf

Summary

Critical vulnerability in InheritanceManager where the nonReentrant modifier uses incorrect transient storage slots, making the reentrancy protection completely ineffective and putting all contract assets at risk.

Vulnerability Details

The nonReentrant modifier in InheritanceManager.sol contains a critical implementation flaw:

modifier nonReentrant() {
assembly {
if tload(1) { revert(0, 0) } // Checks slot 1
tstore(0, 1) // Stores in slot 0
}
_;
assembly {
tstore(0, 0) // Clears slot 0
}
}

Key Issues:

  1. Reentrancy check reads from slot 1 (tload(1))

  2. State is stored in slot 0 (tstore(0, ...))

  3. Since slot 1 is never modified, the check always passes

  4. This makes the reentrancy protection completely ineffective

Impact

  1. Asset Theft

    • Complete draining of ETH through sendETH() or contractInteractions()

    • Theft of any ERC20 tokens through sendERC20()

    • Multiple recursive calls possible during a single transaction

  2. Arbitrary Contract Interaction

    • contractInteractions() allows calls to any external contract

    • Can send ETH with the calls

    • Results are stored in the interactions mapping

    • No effective protection against recursive calls

Tools Used

  • Manual code review

Recommendations

Use same slot for checking and storing:

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

Lead Judging Commences

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

Give us feedback!