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

InheritanceManager::non-reentrant() is an incorrect implementation of a reentrancy lock

vanshikag

Summary

InheritanceManager::non-reentrant() is a low-level implementation of a reentrancy lock using transient storage. This modifier is implemented incorrectly as it uses the wrong storage slot to decide if the function has been previously entered. These functions include sendETH(), contractInteractions() and sendERC20. All three of these are protected by onlyOwner, but they are not non-Reentrant.

modifier nonReentrant() {
assembly {
// reading from transient storage slot 1
if tload(1) { revert(0, 0) }
// storing into slot 0
tstore(0, 1)
}
_;
assembly {
tstore(0, 0)
}
}

Impact

None of the functions with nonReentrant() modifier are protected from reentrancy. They are protected from unauthorized calls, which reduces the impact of this bug.

Proof Of Concept

Add the following to InheritanceManager.t.sol and run tests.

address owner = address(this); // before constructor
uint256 reentrancyCount = 3;
.
.
.
receive() external payable {
if (reentrancyCount > 0) { // attempting to reenter upto 3 times.
im.sendETH(1e18, address(this));
reentrancyCount = reentrancyCount - 1;
}
}
​
function test_reentrancyLock() public {
vm.deal(address(im), 10e18);
vm.startPrank(owner);
im.sendETH(1e18, address(this));
vm.stopPrank();
assertEq(6e18, address(im).balance); // 3 reentrancies plus original transaction.
}

Expected result:

forge test --mt test_reentrancyLock -vvvv
[⠑] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_reentrancyLock() (gas: 68238)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.55ms (3.05ms CPU time)
​
Ran 1 test suite in 791.86ms (7.55ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendation

Bug fix:

- if tload(1) { revert(0, 0) }
+ if tload(0) { revert(0, 0) }
Updates

Lead Judging Commences

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

Wrong value in nonReentrant modifier

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