Open hats-bug-reporter[bot] opened 4 months ago
Thanks, after reviewing it, i decided on medium severity, because funds will not be permanently stuck in the contract.
@0xmahdirostami Im pretty sure this is invalid The reward mechanism can be interpreted in different ways here. If the reward is based on time spend staked in the contract then 9000 tokens might still be valid amount. There is nowhere a claim that says you should receive the full amount of tokens if you jumped in the middle. Also the calculation of the 9000 tokens due to solidity rounding doesnt work because of our precision addition of 1e36. I would say this is invalid
and yes the funds are not stuck because of the paymentprocessor structure
@0xmahdirostami Im pretty sure this is invalid The reward mechanism can be interpreted in different ways here. If the reward is based on time spend staked in the contract then 9000 tokens might still be valid amount. There is nowhere a claim that says you should receive the full amount of tokens if you jumped in the middle. Also the calculation of the 9000 tokens due to solidity rounding doesnt work because of our precision addition of 1e36. I would say this is invalid
Intended behaviours of staking reward distributors like Synthetix are for the rewards to be fully claimable by the respective stakers.
We are not aware of the intended interpretation and was initially told this was not an intended behavior, thus medium.
Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x8547118a13c9ae1841d50875fed4c36cfb31f5497a06c1961a59b3e97b2c874f Severity: high
Description: Description\ The Staking contract of the logic module operates on total amounts of tokens depositd being distributed over a duration, proportianally to user stakes. However the logic contains a Synthetix-like bug, where the period between setting the rewards and the first stake gets left stuck in the contract.
Attack Scenario\ It will be easier to go over a simplified scenario: Let's say the staking begins,
_setRewards
is called with a total amount of 10_000 and a duration of 10_000, thus rewardRate = 1, lastUpdate is set to the timestamp of calling, 0 for simplicity.Now lets say some miniscule time passes and Bob stakes 1000 tokens 10 seconds after the rewards are set. Then we will have:
Let's say nobody else stakes, the period finishes and Bob calls claimRewards(). Logically he should get everything for being the only staker, but:
The period has ended, total rewards was 10_000, but bob only got 9000 of them, even though he was a staker for only 10 seconds later. The other 1000 tokens are stuck inside the contract and cannot be rescued due to lack of such functionality. Even though we can re-add these rewards for a new period, the same bug repeats and there will be dust left aswell, thus High.
Attachments
Proof of Concept (PoC) File
Revised Code File (Optional)
Recommendation\ This is a bug that can be worked around some potential fixes are:
lastUpdate
when rewards are set, instead set it only on the first deposit. This way the cycle starts with the first deposit and not thesetRewards
call