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

[M-2] Incorrect Implementation code in `nonReentrant` Modifier Opens Doors to all kind of Reentrancy Attacks.

agmguindo

Description:

The nonReentrant modifier is implemented incorrectly by checking slot 1 instead of slot 0 in transient storage. This allows reentrancy attacks since the storage slot used for locking is not the expected one.

Although the impact is mitigated in the current implementation by always combining nonReentrant with onlyOwner, this introduces a major security risk if the modifier is later applied to functions that do not require ownership.

modifier nonReentrant() {
assembly {
if tload(1) { // ❌ Incorrect slot check
revert(0, 0)
}
tstore(0, 1)
}
_;
assembly {
tstore(0, 0)
}
}

Impact:

Medium Severity

Likelihood: High – If this modifier is applied to any function that lacks onlyOwner, reentrancy attacks become possible.

Potential Consequences: Attacker-controlled contracts could exploit functions using this incorrect nonReentrant implementation, leading to loss of funds or unintended behavior.

Proof of Concept:

The attacker can re-enter the function before it finishes execution, manipulating contract state.

​
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {InheritanceManager} from "../InheritanceManager.sol";
​
contract Malicious1 {
InheritanceManager inheritance;
​
constructor(address _inheritance) {
inheritance = InheritanceManager(_inheritance);
}
​
receive() external payable {
if (address(inheritance).balance > 0) {
uint256 amount = address(inheritance).balance;
inheritance.sendETH(amount, address(this));
}
}
}
​
​
//Not working because of the ownerModifier
function test_audit_ReentrancyBySendingEth() public {
vm.deal(address(im), 10e18);
vm.startPrank(owner);
im.sendETH(1e18, address(mal));
assertEq(address(im).balance, 0);
assertEq(address(mal).balance, 10e18);
vm.stopPrank();
}
​

We can see we ae reentering the Fn thanks to the vulnerability in nonReentrant modifier but we revert because of onlyOwner.

Logs: [29144] InheritanceManagerTest::test_audit_ReentrancyBySendingEth()

 InheritanceManager::sendETH(1000000000000000000 [1e18], Malicious1: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
 [4640] Malicious1::receive{value: 1000000000000000000}()
 [1310] InheritanceManager::sendETH(9000000000000000000 [9e18], Malicious1: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
 [Revert] NotOwner(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a)

Recommended Mitigation:

To properly implement reentrancy protection, the modifier must use the correct transient storage slot (0):

modifier nonReentrant() {
assembly {
- if tload(1) {
+ 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

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!