Findings
Issue 01
Type | Severity | Location | Status |
Owner Capabilities | High |
| ✔️ Resolved |
Description
The owner of the contract can withdraw users' staked tokens at any time.
Recommendation
If the purpose of this function is to withdraw tokens accidentally sent to the contract, consider adding a require statement that makes sure the withdrawn token by the owner is not the native token. If it is the native token, add a variable that tracks the tokens staked and stops the owner with withdrawing them.
Second Revision
The owner of the contract can modify the token address, and therefore can still withdraw any token.
Issue 02
Type | Severity | Location | Status |
Gas Optimization | Informational |
| ✔️ Resolved |
Description
By default, ETH can't be sent to smart contracts unless they have a receive function.
Recommendation
Consider removing the receive function as it's not necessary.
Issue 03
Type | Severity | Location | Status |
Volatile Code | High |
| ✔️ Resolved |
Description
transferFrom
function can silently fail without being reverted.
Recommendation
Use safeTransferFrom or check the return value of the transferFrom function.
Issue 04
Type | Severity | Location | Status |
Logical Issue | Medium |
| ✔️ Resolved |
Description
The following line can trigger a math underflow if mainPen (that is based on the staked amount) is larger than the reward
Issue 05
Type | Severity | Location | Status |
Logical Issue | Informational |
| ✔️ Resolved |
Description
In emergencyWithdraw function, stakedAmount is not decreased.
Issue 06
Type | Severity | Location | Status |
Logical Issue | Medium |
| ✔️ Resolved |
According to the docs if
The equation for calculating the rewards is wrong.
Issue 07
Type | Severity | Location | Status |
Best Practice | Low |
| ❌ Not Resolved |
Description
Penalty and APY percentages use a 1e18 multiplier, that allows for extremely high precision. However, it is customary to use a lower multiplier like 100 or 1,000, to minimize the risk of human error.
Recommendation
We suggest reducing the 1e18 multiplier to 1,000, which provides a precision of up to 0.1%
Issue 08
Type | Severity | Location | Status |
Best Practice | Low |
| ❌ Not Resolved |
Description
It is bad practice to duplicate core functionality of the code (e.g. APY calculation).
Recommendation
Use only one function that calculates the APY and calls it with the appropriate parameters every time.
We recommend using one calculate APY function that receives:
amount, apy, duration.
Issue 09
Type | Severity | Location | Status |
Logical Issue | High |
| ✔️ Resolved |
Description
If a pool has no mainPenalty, mainAmount will be 0 which means the user won't be able to withdraw his original staked amount.
General Notes
Consider adding an emergency withdrawal function that would let the user simply withdraw his staked tokens without doing extra logic, even if the token is paused. Note that if this suggestion is accepted, it may allow users to avoid penalties by calling emergencyWithdraw - The team has decided to enable emergency withdrawals only when the staking contract is paused.
isFinished modifier checks if the staker status is not finished, consider renaming the function to reflect this logic.
The code does not support a token with transfer fees - staker.amount and the reserves of the pool may not be correct which will every function that withdraws it.
Last updated