Summary

The nonReentrant modifier in the AppStorage.sol contract may lead to locking of the contract, rendering functions utilizing the modifier unusable, if a revert occurs after setting the reentrantStatus. This is a serious issue that could lead to a denial of service (DoS) attack on the contract.

Vulnerability Details

The nonReentrant modifier in the contract is designed to prevent reentrancy attacks by checking the reentrantStatus. If it's set to ENTERED, the function is immediately reverted. If not, the reentrantStatus is set to ENTERED and the function proceeds. Upon completion, the reentrantStatus is reset to NOT_ENTERED. However, a vulnerability exists where if an exception occurs after the function logic (indicated by the _; statement in the modifier), the reentrantStatus remains as ENTERED due to a revert (caused by a failed requirement or an unsuccessful external call). This leaves the contract in a locked state, barring further calls to any function using the nonReentrant modifier. Consequently, users would be unable to deposit or withdraw tokens until the issue is resolved.

Impact

Contract Locking: Functions protected by the nonReentrant modifier can become indefinitely locked, making it impossible for users to perform crucial actions, such as depositing or withdrawing tokens.

Loss of User Funds: In a scenario where the contract is locked and there's no method for recovery, users might be unable to access or manage their funds, potentially leading to a permanent loss.

Potential Lock Points:

1.External calls like IERC20(zeth).burnFrom(msg.sender, amount); or IERC20(asset).mint(msg.sender, amount); in the functions can potentially fail for various reasons (lack of approval, insufficient balance, etc.).

2.Arithmetic operations or state checks, e.g., s.assetUser[asset][msg.sender].ercEscrowed += amount; or if (amount > AssetUser.ercEscrowed) revert Errors.InsufficientERCEscrowed(); could also cause reverts if the conditions aren't met.

3. Risk of Unintended Behavior: If the contract becomes locked due to the issue with the nonReentrant modifier, users could lose access to their funds temporarily or even permanently, depending on the situation and how the contract is set up for upgrades or fixes.

Tools Used

Manual Code Review.

Recommendations

Consider using a well-tested external library for reentrancy protection, like OpenZeppelin's ReentrancyGuard, which has been audited and widely adopted in the community.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.21;

// Reentrancy state
uint256 internal constant NOT_ENTERED = 1;
uint256 internal constant ENTERED = 2;

// Custom error for reentrant calls
error ReentrantCall();

struct AppStorage {
uint256 reentrantStatus;
}

contract Modifiers {
AppStorage internal s;

modifier nonReentrant() {
    _nonReentrantBefore();
    _;
    _nonReentrantAfter();
}

function _nonReentrantBefore() internal {
    if (s.reentrantStatus == ENTERED) {
        revert ReentrantCall();
    }
    s.reentrantStatus = ENTERED;
}

function _nonReentrantAfter() internal {
    s.reentrantStatus = NOT_ENTERED;
}

}