两用户单方法互换ERC20的重入防护问题及安全实现方案咨询

First, let's break down why the nonReentrant modifier isn't resolving your problem: your current code violates the Checks-Effects-Interactions security pattern. Even with reentrancy protection, state updates (like modifying liquidity) happen after external calls (transferFrom), leaving your contract vulnerable to exploits where stale state data is used during reentrant or concurrent operations.

Key Problems in Your Current Code #

1. State updates after external calls: You subtract from liquidity after executing token transfers. If an attacker's contract triggers a reentrant call (even if nonReentrant blocks this function, they might target other functions relying on the unupdated liquidity value), they could exploit the stale state.
2. Unverified user balances: The transferFrom calls will fail if the user doesn't have enough tokens, but adding an explicit pre-check improves clarity and prevents unnecessary gas usage.

Secure Implementation Using Checks-Effects-Interactions #

Rewrite the function to follow secure practices:

1. Checks: Validate all conditions (including user balances) before any state changes.
2. Effects: Update all state variables before making external calls.
3. Interactions: Execute token transfers only after state is fully updated.

function swapTokenToEvolve(uint256 _tokenAmount, uint256 _stageIndex)
    public
    checkStageTime(_stageIndex)
    checkRemainingAmount(_tokenAmount, _stageIndex)
    nonReentrant
    returns (bool)
{
    // Checks: Calculate amounts and verify user has sufficient tokens
    uint256 tokenPrice = salesStages[_stageIndex].price;
    uint256 stableTokenAmount = multiply(_tokenAmount, tokenPrice, decimal);
    
    require(
        IERC20(token).balanceOf(_msgSender()) >= stableTokenAmount,
        "Insufficient token balance"
    );

    // Effects: Update state BEFORE external transfers
    salesStages[_stageIndex].liquidity = salesStages[_stageIndex].liquidity.sub(_tokenAmount);

    // Interactions: Execute token transfers
    require(
        IERC20(currencyToken).transferFrom(owner(), _msgSender(), _tokenAmount),
        "Currency transfer failed"
    );
    require(
        IERC20(token).transferFrom(_msgSender(), owner(), stableTokenAmount),
        "Token transfer failed"
    );

    return true;
}

Additional Critical Security Steps #

• Use OpenZeppelin's ReentrancyGuard: Ensure your contract inherits from ReentrancyGuard (from @openzeppelin/contracts/security/ReentrancyGuard.sol). Rolling your own mutex implementation is error-prone—rely on battle-tested code.
• Validate calculations: If using Solidity <0.8.0, use SafeMath to prevent overflow/underflow. For Solidity 0.8+, built-in checks handle this, but you can add explicit bounds checks if needed.
• Confirm transfer logic: Double-check that the transfer directions align with your intended swap flow (user sends token to receive currencyToken, or vice versa).

Why This Fix Works #

By moving state updates before external calls, you ensure that even if an attacker triggers a reentrant call, the state reflects the latest changes. The nonReentrant modifier blocks reentry into this function, and the updated state prevents other functions from acting on stale data.

内容的提问来源于stack exchange，提问作者Mahdi Ashouri