Core Contracts

Summary

The function collectFee in FeeCollector.sol does not follow the Checks-Effects-Interactions (CEI) principle, which is a best practice in Solidity to prevent reentrancy and other vulnerabilities. Although the function is protected by the nonReentrant modifier, it still performs an external call (a token transfer) before updating the contract's internal state. This can lead to potential issues, including reentrancy vulnerabilities if nonReentrant is removed in future updates or interactions with untrusted tokens.

Vulnerability Details

In FeeCollector.sol#L162, the collectFee function is responsible for collecting fees in raacToken. However, it does not follow the CEI pattern, as it interacts with an external contract before updating internal state variables.

The function calls an external contract (safeTransferFrom) before updating the contract's internal state. Although the nonReentrant modifier prevents direct reentrancy attacks, it is still considered best practice to follow the CEI principle to minimize risks.

Impact

If the function is modified in the future and nonReentrant is removed or bypassed, an attacker could exploit this issue to reenter the contract before the state is updated. Additionally, interacting with an untrusted ERC20 token that has custom logic in transferFrom could cause unpredictable behavior, leading to incorrect fee tracking or failed transactions.

Tools Used

Manual code review

Recommendations

To follow the CEI principle, the function should first update the contract’s internal state and then perform the external token transfer. This ensures that even if an external call behaves unexpectedly, the contract remains in a consistent state.

The updated version is shown below:

By making this change, the contract ensures that its internal state is consistent before making any external calls, reducing risks associated with external dependencies.