[Low] Re-Entrancy Vulnerability Due to Non-Aligned Best Practices in Withdrawal Logic

Low/Info issue submitted by ygoel72

Summary

Under the withdraw(uint256 _pid, uint256 _withdrawAmount), user.rewardDebt is updated after the safeTransfer() external call. While this does not currently pose a risk, it is not aligned with best practices of check-effect-interations and opens your code to a potential re-entrancy attack in the future

Vulnerability Detail

The vulnerability stems from the order of operations in the withdrawal function. After transferring tokens to the user, the function updates the rewardDebt of the user. This sequence of actions could potentially allow malicious contracts to exploit the re-entrancy vulnerability.

Impact

While the current implementation does not pose a direct financial threat, adhering strictly to best practices is crucial for preventing future vulnerabilities. Disregarding the Checks-Effects-Interactions pattern increases the risk of re-entrancy attacks, which could compromise the contract's security and integrity

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L735-L739

Tool used

Manual Review

Recommendation

To align with best practices and mitigate the risk of re-entrancy attacks, the rewardDebt update should precede the external call.

+        user.rewardDebt = userAmount *
+          pool.accPointsPerShare /
+              1e18;
        pool.lpToken.safeTransfer(msg.sender, _withdrawAmount);

-        user.rewardDebt = userAmount *
-          pool.accPointsPerShare /
-              1e18;

        emit Withdraw(msg.sender, _pid, _withdrawAmount);
    }

sherlock-audit / 2024-05-sophon-judging