Permanent loss of rewards on temporary underfunding of RewardsManager contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

L815 in the _transferAjnaRewards() method of the RewardsManager contract silently trims every reward, that is about to be transferred, down to the actual balance of the contract instead of reverting the transaction due to lack of funds. It's clear that this was added with good intentions and is only a problem when the RewardsManager contract is underfunded, but the consequences are severe nevertheless.

_transferAjnaRewards() is called at 4 instances in the RewardsManager contract but the consequences become clearest through the claimRewards() and further _claimRewards() methods. Both of these methods take measures that the same rewards can only be claimed once, see L122 and L594.

Therefore calling claimRewards() during a period where the RewardsManager contract is underfunded leads to the following outcomes:
1: Rewards are silently trimmed before transfer -> receive less than expected without revert
2: Event ClaimRewards() is emitted as if full rewards were paid -> misleading the front-end
3: Transaction succeeds, internal accounting "thinks" rewards were claimed -> re-try to claim rewards reverts with AlreadyClaimed() error

The result is a permanent loss of rewards (up to the specified epoch) for the user while pretending the rewards were successfully claimed via event (log).

Proof of Concept

The following PoC, based on an existing test case, demonstrates that a user, who tries to claim rewards during temporary underfunding, gets less than expected rewards while being misled by the ClaimRewards() event. When re-attempting to claim the full rewards, the AlreadyClaimed() error arises, i.e. permanent loss up to the specified epoch.

Just apply the diffs below and run the modified test case with forge test --match testMultiPeriodRewardsSingleClaimLoss:

diff --git a/ajna-core/tests/forge/unit/Rewards/RewardsDSTestPlus.sol b/ajna-core/tests/forge/unit/Rewards/RewardsDSTestPlus.sol
index 93fe062..ef59c1d 100644
--- a/ajna-core/tests/forge/unit/Rewards/RewardsDSTestPlus.sol
+++ b/ajna-core/tests/forge/unit/Rewards/RewardsDSTestPlus.sol
@@ -167,6 +167,25 @@ abstract contract RewardsDSTestPlus is IRewardsManagerEvents, ERC20HelperContrac
         assertEq(_ajnaToken.balanceOf(from), fromAjnaBal + reward);
     }

+    function _claimRewardsExt(
+        address from,
+        address pool,
+        uint256 tokenId,
+        uint256 eventReward,
+        uint256 actualReward,
+        uint256[] memory epochsClaimed
+    ) internal {
+        changePrank(from);
+        uint256 fromAjnaBal = _ajnaToken.balanceOf(from);
+
+        uint256 currentBurnEpoch = IPool(pool).currentBurnEpoch();
+        vm.expectEmit(true, true, true, true);
+        emit ClaimRewards(from, pool, tokenId, epochsClaimed, eventReward);
+        _rewardsManager.claimRewards(tokenId, currentBurnEpoch);
+
+        assertEq(_ajnaToken.balanceOf(from), fromAjnaBal + actualReward);
+    }
+
     function _moveStakedLiquidity(
         address from,
         uint256 tokenId,
diff --git a/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol b/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
index 4100e9f..a4a1c25 100644
--- a/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
+++ b/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
@@ -911,7 +911,7 @@ contract RewardsManagerTest is RewardsHelperContract {
         });
     }

-    function testMultiPeriodRewardsSingleClaim() external {
+    function testMultiPeriodRewardsSingleClaimLoss() external {
         skip(10);

         uint256 totalTokensBurned;
@@ -1090,16 +1090,30 @@ contract RewardsManagerTest is RewardsHelperContract {
         rewardsEarned = _rewardsManager.calculateRewards(tokenIdOne, _pool.currentBurnEpoch());
         assertEq(rewardsEarned, 491.127945245927630407 * 1e18);

-        // claim all rewards accrued since deposit
-        _claimRewards({
+
+        // RewardsManager is temporarily out of Ajna tokens
+        deal(address(_ajnaToken), address(_rewardsManager), 0);
+
+        // claim all rewards accrued since deposit -> event (log) pretends we got reward, but we didn't
+        _claimRewardsExt({
             pool:          address(_pool),
             from:          _minterOne,
             tokenId:       tokenIdOne,
             epochsClaimed: _epochsClaimedArray(5,0),
-            reward:        491.127945245927630407 * 1e18
+            eventReward:   rewardsEarned, // expect full rewards according to event (log)
+            actualReward:  0              // expect 0 actual rewards, should be: rewardsEarned
         });
-        assertEq(_ajnaToken.balanceOf(_minterOne), rewardsEarned);
+        assertEq(_ajnaToken.balanceOf(_minterOne), 0); // got 0 rewards, should be: rewardsEarned
         assertLt(rewardsEarned, Maths.wmul(totalTokensBurned, 0.800000000000000000 * 1e18));
+
+        // RewardsManager is solvent again */
+        deal(address(_ajnaToken), address(_rewardsManager), 100_000_000 * 1e18);
+
+        // re-try to claim all rewards accrued since deposit -> error "AlreadyClaimed", user lost rewards
+        _assertAlreadyClaimedRevert({
+            from:    _minterOne,
+            tokenId: tokenIdOne
+        });
     }

     function testMoveStakedLiquidity() external {

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Since the internal accounting is not suited to handle partial claims, I recommend to let the transaction fail on lack of funds, see the diff below. This way, the user does not lose the eligibility to claim rewards and can try again once the RewardsManager is properly funded.

diff --git a/ajna-core/src/RewardsManager.sol b/ajna-core/src/RewardsManager.sol
index 314b476..54471ae 100644
--- a/ajna-core/src/RewardsManager.sol
+++ b/ajna-core/src/RewardsManager.sol
@@ -805,15 +805,9 @@ contract RewardsManager is IRewardsManager, ReentrancyGuard {

     /** @notice Utility method to transfer `Ajna` rewards to the sender
      *  @dev   This method is used to transfer rewards to the `msg.sender` after a successful claim or update.
-     *  @dev   It is used to ensure that rewards claimers will be able to claim some portion of the remaining tokens if a claim would exceed the remaining contract balance.
      *  @param rewardsEarned_ Amount of rewards earned by the caller.
      */
     function _transferAjnaRewards(uint256 rewardsEarned_) internal {
-        // check that rewards earned isn't greater than remaining balance
-        // if remaining balance is greater, set to remaining balance
-        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
-        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;
-
         if (rewardsEarned_ != 0) {
             // transfer rewards to sender
             IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);

Assessed type

Token-Transfer

[c4-judge]

Picodes marked the issue as duplicate of #361

[c4-judge]

Picodes marked the issue as satisfactory