RewardsManager doesn't delete old bucket snapshot info on unstaking
Summary
RewardsManager's unstake() use delete stakes[tokenId_] to clear old stake state, but snapshot is the nested mapping in the StakeInfo structure and will not be reset this way as delete operation do not traverse through nested mappings as it lacks key set information.
Vulnerability Detail
stakes[tokenId_] gets written on staking and mapping(uint256 => BucketState) snapshot is written for the current list of buckets. This means if this list persists and there were no bucket changes it's ok as new values will be overwritten on next stake.
But, if Bob the staker has changed his composition of buckets and his second stake takes place over another set, possibly intersecting with the first one, old part will persist. If then Bob's positionIndexes = positionManager.getPositionIndexes(tokenId_) changed after the second stake, say as a result of PositionManager's moveLiquidity(), and indices from the first set were added there, their snapshot values from the first stake will be reused.
Impact
If Bob knows this it will be straightforward for him to exploit the mechanics, obtaining extra rewards (interest earned will be counted from the first stake time for old positions) at the expense of other stakers.
struct StakeInfo {
address ajnaPool; // address of the Ajna pool the NFT corresponds to
uint96 lastInteractionBurnEpoch; // last burn event the stake interacted with the rewards contract
address owner; // owner of the LP NFT
uint96 stakingEpoch; // epoch at staking time
mapping(uint256 => BucketState) snapshot; // the LP NFT's balances and exchange rates in each bucket at the time of staking
}
struct BucketState {
uint256 lpsAtStakeTime; // [RAY] LP amount the NFT owner is entitled in current bucket at the time of staking
uint256 rateAtStakeTime; // [RAY] current bucket exchange rate at the time of staking (RAY)
}
Both are written on staking, but only for the list of indices as of time of staking:
uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);
for (uint256 i = 0; i < positionIndexes.length; ) {
uint256 bucketId = positionIndexes[i];
BucketState storage bucketState = stakeInfo.snapshot[bucketId];
// record the number of lp tokens in bucket at the time of staking
bucketState.lpsAtStakeTime = positionManager.getLPTokens(
tokenId_,
bucketId
);
// record the bucket exchange rate at the time of staking
bucketState.rateAtStakeTime = IPool(ajnaPool).bucketExchangeRate(bucketId);
// iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
unchecked { ++i; }
}
rateAtStakeTime and lpsAtStakeTime are used for the accrued interest calculation in the _calculateNextEpochRewards():
uint256 bucketRate;
if (epoch_ != stakingEpoch_) {
// if staked in a previous epoch then use the initial exchange rate of epoch
bucketRate = bucketExchangeRates[ajnaPool_][bucketIndex][epoch_];
} else {
// if staked during the epoch then use the bucket rate at the time of staking
bucketRate = bucketSnapshot.rateAtStakeTime;
}
// calculate the amount of interest accrued in current epoch
uint256 interestEarned = _calculateExchangeRateInterestEarned(
ajnaPool_,
nextEpoch,
bucketIndex,
bucketSnapshot.lpsAtStakeTime,
bucketRate
);
This happens for the current positionIndexes = positionManager.getPositionIndexes(tokenId_):
function _calculateAndClaimRewards(
uint256 tokenId_,
uint256 epochToClaim_
) internal returns (uint256 rewards_) {
address ajnaPool = stakes[tokenId_].ajnaPool;
uint256 lastBurnEpoch = stakes[tokenId_].lastInteractionBurnEpoch;
uint256 stakingEpoch = stakes[tokenId_].stakingEpoch;
uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);
// iterate through all burn periods to calculate and claim rewards
for (uint256 epoch = lastBurnEpoch; epoch < epochToClaim_; ) {
uint256 nextEpochRewards = _calculateNextEpochRewards(
tokenId_,
epoch,
stakingEpoch,
ajnaPool,
positionIndexes
);
Say Bob restaked, the snapshot persisted. Then if positions changed since the second stake and new indices have been there before (i.e. old ones were readded, so they weren't reset on the second stake() as were added later, but their values end up not being void as they were there on the first stake and persisted), then their values will be reused from the first Bob's staking.
/**
* @notice Calculate the amount of interest that has accrued to a lender in a bucket based upon their LPs.
* @param pool_ Address of the pool whose exchange rates are being checked.
* @param nextEventEpoch_ The next event epoch to check the exchange rate for.
* @param bucketIndex_ Index of the bucket to check the exchange rate for.
* @param bucketLPs Amount of LPs in bucket.
* @param exchangeRate_ Exchange rate in current epoch.
* @return interestEarned_ The amount of interest accrued.
*/
function _calculateExchangeRateInterestEarned(
address pool_,
uint256 nextEventEpoch_,
uint256 bucketIndex_,
uint256 bucketLPs,
uint256 exchangeRate_
) internal view returns (uint256 interestEarned_) {
if (exchangeRate_ != 0) {
uint256 nextExchangeRate = bucketExchangeRates[pool_][bucketIndex_][nextEventEpoch_];
// calculate interest earned only if next exchange rate is higher than current exchange rate
if (nextExchangeRate > exchangeRate_) {
// calculate the equivalent amount of quote tokens given the stakes lp balance,
// and the exchange rate at the next and current burn events
interestEarned_ = Maths.rayToWad(Maths.rmul(nextExchangeRate - exchangeRate_, bucketLPs));
}
}
}
Tool used
Manual Review
Recommendation
IRewardsManagerState.BucketState doesn't contain any nested structures, so delete bucketState will reset it fully:
struct BucketState {
uint256 lpsAtStakeTime; // [RAY] LP amount the NFT owner is entitled in current bucket at the time of staking
uint256 rateAtStakeTime; // [RAY] current bucket exchange rate at the time of staking (RAY)
}
Consider clearing the current stake snapshots on unstaking, for example:
hyh
high
RewardsManager doesn't delete old bucket snapshot info on unstaking
Summary
RewardsManager's unstake() use
delete stakes[tokenId_]
to clear old stake state, butsnapshot
is the nested mapping in theStakeInfo
structure and will not be reset this way as delete operation do not traverse through nested mappings as it lacks key set information.Vulnerability Detail
stakes[tokenId_]
gets written on staking andmapping(uint256 => BucketState) snapshot
is written for the current list of buckets. This means if this list persists and there were no bucket changes it's ok as new values will be overwritten on next stake.But, if Bob the staker has changed his composition of buckets and his second stake takes place over another set, possibly intersecting with the first one, old part will persist. If then Bob's
positionIndexes = positionManager.getPositionIndexes(tokenId_)
changed after the second stake, say as a result of PositionManager's moveLiquidity(), and indices from the first set were added there, their snapshot values from the first stake will be reused.Impact
If Bob knows this it will be straightforward for him to exploit the mechanics, obtaining extra rewards (interest earned will be counted from the first stake time for old positions) at the expense of other stakers.
Code Snippet
RewardsManager's unstake() deletes
stakes[tokenId_]
:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L187-L203
stakes[tokenId_]
isStakeInfo
structure:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L76
https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L21
It contains
snapshot
mapping elemewnt that will not be cleared on delete:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/interfaces/rewards/IRewardsManagerState.sol#L56-L62
Per operation docs:
https://docs.soliditylang.org/en/latest/types.html#delete
So if you delete a struct, it will reset all members that are not mappings and also recurse into the members unless they are mappings
This way restaking the
tokenId_
will reuse the oldsnapshot
mapping.BucketState
structure consists ofrateAtStakeTime
andlpsAtStakeTime
:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/interfaces/rewards/IRewardsManagerState.sol#L64-L67
Both are written on staking, but only for the list of indices as of time of staking:
https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L144-L162
rateAtStakeTime
andlpsAtStakeTime
are used for the accrued interest calculation in the _calculateNextEpochRewards():https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L333-L351
This happens for the current
positionIndexes = positionManager.getPositionIndexes(tokenId_)
:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L272-L292
Say Bob restaked, the
snapshot
persisted. Then if positions changed since the second stake and new indices have been there before (i.e. old ones were readded, so they weren't reset on the second stake() as were added later, but their values end up not being void as they were there on the first stake and persisted), then their values will be reused from the first Bob's staking.This will expand Bob's interest earned reading:
https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L368-L398
Tool used
Manual Review
Recommendation
IRewardsManagerState.BucketState doesn't contain any nested structures, so
delete bucketState
will reset it fully:https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/interfaces/rewards/IRewardsManagerState.sol#L64-L67
Consider clearing the current stake snapshots on unstaking, for example:
https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/RewardsManager.sol#L187-L203