Ajna Protocol - ABAIKUNANBAEV's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L811

Vulnerability details

Impact

When calling stake() or unstake() or moveStakedLiquidity(), eventually the functions will call interanl transferAjnaRewards() that will transfer user's rewards. The problem is that the function relies on the balanceOf() of the ajnaToken. And there is if statement that checks if rewardsEarned > ajnaBalance. If so, the rewards will be equal to the ajnaBalance. This logic can be violated and eventually lead to the user getting less rewards.

Proof of Concept

Imagine the following scenario:

1. ajnaBalance = 1000.
2. Alice decides to unstake() his tokens and get his rewards (900 tokens, for example)
3. Bob with 700 reward tokens sees this transaction and decides to front-run. He unstakes his 700 tokens and now the ajnaBalance equals to 300 tokens. And when Alice's tx executes, she'll get rewardsEarned_ = ajnaBalance leaving her with less rewards than she earned.

Tools Used

Manual review.

Recommended Mitigation Steps

Add some custom logic to hide the rewardsEarned_ by the user making it more difficult target for attackers. Also the statement: if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance can be changed to avoid situation where rewards earned bigger than the actual token balance (maybe minBalance can be introduced).

Assessed type

MEV

Lines of code

claimRewards(): https://github.com/ajna-finance/ajna-core/blob/main/src/RewardsManager.sol#L122 moveStakedLiquiidty(): https://github.com/ajna-finance/ajna-core/blob/main/src/RewardsManager.sol#L152 unstake(): https://github.com/ajna-finance/ajna-core/blob/main/src/RewardsManager.sol#L280

Vulnerability details

Impact

When we call external claimRewards(), we need to check whether msg.sender == stakeInfo.owner and whether the rewards have been claimed for this epoch and only after that call internal _claimRewards. However, when calling different functions such as moveStakedLiquidity() and unstake() we check only if msg.sender == stakeInfo.owner forgetting about isEpochClaimed mapping before calling internal _claimRewards. This allows the users to claim rewards multiple times in the same rewards by moving liquidity and staking / unstaking tokenId.

Proof of Concept

1. Alice stakes her tokenId and starts earning the rewards.
2. She claims her rewards by calling claimRewards().
3. She then calls moveStakedLiquidity() and claims her rewards again in the same epoch.

Tools Used

Manual review.

Recommended Mitigation Steps

The same check as in the claimRewards() for isEpochClaimed should be added to moveStakedLiquidity() and unstake().

Assessed type

Other

Unchecked return value on add() function in PositionManager.sol:

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#186

Although it doesn't seem to break the protocol and cause any unexpected behavior, it's better to handle the logic on the functions that doesn't revert on failure and return false silently. add() function from Enumerable.sol library will return false if the index is already in the set, but it doesn't revert and continue execution even if it's on the list.