Staking rewards can be sandwichable

Summary

Staking rewards can be sandwiched because there is no fee or lockup period on stakin/unstaking

Vulnerability Detail

There exists no fee or lock-up period associated to staking to receive a portion of the rewards which if a an attacker can profit from by sandwiching they staking and unstaking and profit from the updated exchange rate on the bucket and the epoch: https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/RewardsManager.sol#L196

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/RewardsManager.sol#L153

Impact

Attacker can profit from staking and unstaking a tokenId in the same block because there is no fee or lockup period to prevent it

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/RewardsManager.sol#L153

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/RewardsManager.sol#L770

Tool used

Manual Review

Recommendation

Consider implementing a staking and unstaking fee or a "warm up" period where stakers can't accrue rewards

grandizzy commented 1 year ago

That's operating per design - if stake and unstake then the rewards for that period of time / exchange rate difference is calculated. There's also the constraint that once unstaked one could not stake again and claim rewards for the epoch.

0xffff11 commented 1 year ago

Users should not be able to game the staking rewards. That is why it is very common to have either staking/unstaking fees and cooldown periods for it not to happen. I still see the issue as valid. cc @grandizzy

grandizzy commented 1 year ago

Users should not be able to game the staking rewards. That is why it is very common to have either staking/unstaking fees and cooldown periods for it not to happen. I still see the issue as valid. cc @grandizzy

@0xffff11 sorry my comments were misleading The design doesn't allow such as one could only claim rewards for previous epochs and not for the current one. So if they stake in epoch 2 for example, they won't be able to claim rewards for epoch 2 but they could only wait to get in epoch 3 in order to claim rewards for epoch 2, let me explain why:

when staking we set lastClaimedEpoch to current epoch https://github.com/ajna-finance/contracts/blob/develop/src/RewardsManager.sol#L170 (so will be 2)
```
    // initialize last time interaction at staking epoch
    stakeInfo.lastClaimedEpoch = curBurnEpoch;
```

when unstaking the _calculateAndClaimAllRewards function is called, passing current epoch as parameter https://github.com/ajna-finance/contracts/blob/develop/src/RewardsManager.sol#L743

        rewardsEarned = _calculateAndClaimAllRewards(
            stakeInfo,
            tokenId_,
            IPool(ajnaPool).currentBurnEpoch(),
            false,
            ajnaPool
        );

_calculateAndClaimAllRewards function further calls _calculateAndClaimStakingRewards function to get staking rewards https://github.com/ajna-finance/contracts/blob/develop/src/RewardsManager.sol#L528

in _calculateAndClaimStakingRewards the rewards are calculated from last claimed epoch (in our case 2) only to epoch < than epoch to claim (in our case 2 as well) https://github.com/ajna-finance/contracts/blob/develop/src/RewardsManager.sol#L340

    uint256 lastClaimedEpoch = stakes[tokenId_].lastClaimedEpoch;
    uint256 stakingEpoch     = stakes[tokenId_].stakingEpoch;

    uint256[] memory positionIndexes = positionManager.getPositionIndexesFiltered(tokenId_);

    // iterate through all burn periods to calculate and claim rewards
    for (uint256 epoch = lastClaimedEpoch; epoch < epochToClaim_; ) {

so since lastClaimedEpoch is 2 and epochToClaim_ is 2 as well the for loop won't enter / no rewards will be calculated

PR with unit tests that proves one cannot claim for current epoch: https://github.com/ajna-finance/contracts/pull/913 Hence we consider this report invalid