The provided Solidity code implements a rewards pool contract for a wrapped liquid staking token. While the code is generally structured well, there are several potential vulnerabilities and considerations you should be aware of:
Although the use of SafeERC20 provides some safety during token transfers, the _withdraw function can be susceptible to reentrancy attacks due to the external call to wsdToken.unwrap(toWithdraw) and the transfer to _account. To mitigate this:
Use the Checks-Effects-Interactions pattern. Update state variables (like userRewards[_account] and totalRewards) before making any external calls.
totalRewardsIn the distributeRewards function, totalRewards is updated with the newly distributed rewards without checking for underflows or ensuring that toDistribute is non-negative. This could lead to unintended consequences if the logic of balanceOf or other parts of the contract is flawed.
Ensure that toDistribute is correctly calculated and non-negative.
Consider using SafeMath for arithmetic operations, especially if you're working with older versions of Solidity.
The emit DistributeRewards event is emitted after the state is modified, which is generally acceptable. However, it may be beneficial to include more context in events, such as balance before distribution, to facilitate easier off-chain tracking.
The distributeRewards function is public, which means anyone can call it. This can lead to abuse if users can continuously trigger distributions without any checks.
Consider adding access control (e.g., only allowing the controller or specific users to call this function).
withdrawableRewards LogicThe withdrawableRewards function directly calls the getUnderlyingByWrapped function, which could potentially lead to inaccurate reward calculations if there are discrepancies in the wrapped and unwrapped token values.
Ensure that the getUnderlyingByWrapped function is reliable and accounts for any edge cases, such as rounding errors or discrepancies in token valuations.
In Solidity 0.8.x and above, overflow and underflow are automatically checked, but in cases where you're using external libraries or handling user inputs, you may want to explicitly check for these.
updateReward LogicThe logic in updateReward does not handle scenarios where the withdrawableRewardsWrapped could be less than userRewards[_account], potentially leading to negative balances.
Implement checks to ensure that newRewards does not cause inconsistencies.
Input parameters (such as addresses in the constructor) are not validated. If invalid addresses are passed, it could lead to undesired behavior.
Add checks to validate addresses and ensure they are not zero addresses.
Although SafeERC20 handles the potential failure of token transfers, it’s still good practice to check if the token.transfer and wsdToken.unwrap calls succeed.
Consider wrapping these calls in require statements to ensure success.
Here's an example of how you might refactor the _withdraw function to mitigate some of these issues:
While the code is mostly sound, addressing the points above can help enhance the security and reliability of the contract. Always conduct thorough testing, consider using formal verification methods, and potentially engage in security audits for critical contracts handling user funds.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.