# Wrong reward distribution between early and late depositors because of the late `syncRewards()` call in the cycle, `syncReward()` logic should be executed in each withdraw or deposits (without reverting) Medium · Code4rena · Avalanche liquid staking · 15th December 2022 Finding M-15 of the GoGoPool competition. - Protocol: https://code4rena.com/audits/2022-12-gogopool-contest - Codebase: https://github.com/0xsimao/2022-12-gogopool/tree/aec9928d8bdce8a5a4efe45f54c39d4fc7313731 - Source: https://github.com/code-423n4/2022-12-gogopool-findings/issues/478 ---

Function `syncRewards()` distributes rewards to TokenggAVAX holders, it linearly distribute cycle's rewards from `block.timestamp` to the cycle end time which is next multiple of the `rewardsCycleLength` (the end time of the cycle is defined and the real duration of the cycle changes). when a cycle ends `syncRewards()` should be called so the next cycle starts but if `syncRewards()` doesn't get called fast, then users depositing or withdrawing funds before call to `syncRewards()` would lose their rewards and those rewards would go to users who deposited funds after `syncRewards()` call. contract should try to start the next cycle whenever deposit or withdraw happens to make sure rewards are distributed fairly between users. ### Proof of Concept This is `syncRewards()` code: function syncRewards() public { uint32 timestamp = block.timestamp.safeCastTo32(); if (timestamp < rewardsCycleEnd) { revert SyncError(); } uint192 lastRewardsAmt_ = lastRewardsAmt; uint256 totalReleasedAssets_ = totalReleasedAssets; uint256 stakingTotalAssets_ = stakingTotalAssets; uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_; // Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`. uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; lastRewardsAmt = nextRewardsAmt.safeCastTo192(); lastSync = timestamp; rewardsCycleEnd = nextRewardsCycleEnd; totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_; emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt); } As you can see whenever this function is called it starts the new cycle and sets the end of the cycle to the next multiple of the `rewardsCycleLength` and it release the rewards linearly between current timestamp and cycle end time. So if `syncRewards()` get called near to multiple of the `rewardsCycleLength` then rewards would be distributed with higher speed in less time. The problem is that users depositing funds before call `syncRewards()` won't receive new cycles rewards and early depositing won't get considered in reward distribution if deposits happen before `syncRewards()` call and if a user withdraws his funds before the `syncRewards()` call then he receives no rewards. Imagine this scenario: 1. `rewardsCycleLength` is 10 days and the rewards for the next cycle is `100` AVAX. 2. the last cycle has been ended and user1 has `10000` AVAX deposited and has 50% of the pool shares. 3. `syncRewards()` don't get called for 8 days. 4. users1 withdraws his funds receive `10000` AVAX even so he deposits for 8 days in the current cycle. 5. users2 deposit `1000` AVAX and get 10% of pool shares and the user2 would call `syncRewards()` and contract would start distributing `100` avax as reward. 6. after 2 days cycle would finish and user2 would receive `100 * 10% = 10` AVAX as rewards for his `1000` AVAX deposit for 2 days but user1 had `10000` AVAX for 8 days and would receive 0 rewards. So rewards won't distribute fairly between depositors across the time and any user interacting with contract before the `syncRewards()` call can lose his rewards. Contract won't consider deposit amounts and duration before `syncRewards()` call and it won't make sure that `syncRewards()` logic would be executed as early as possible with deposit or withdraw calls when a cycle ends. ### Tools Used VIM ### Recommended Mitigation Steps One way to solve this is to call `syncRewards()` logic in each deposit or withdraw and make sure that cycles start as early as possible (the revert "SyncError()" in the `syncRewards()` should be removed for this). --- Related findings: - [Cds amounts to reduce from each chain are incorrect and will lead to the inability to withdraw cds in one of the chains](https://0xsimao.com/findings/autonomint-reduce-each-inability-withdraw): Autonomint - [`Claimers` can receive less `feePerClaim` than they should if some prizes are already claimed or if reverts because of a reverting hook](https://0xsimao.com/findings/pooltogether-the-prize-layer-for-defi-claimers-fee-reverts-hook): PoolTogether Prize Layer - [Users redeeming early will withdraw `Ra` without decreasing the amount locked, which will lead to stolen funds when withdrawing after expiry](https://0xsimao.com/findings/cork-protocol-redeeming-withdraw-locked-stolen): Cork Protocol - [Withdrawals can fail due to deposits reverting in `completeQueuedWithdrawal()`](https://0xsimao.com/findings/renzo-withdrawals-deposits-complete-withdrawal): Renzo