Ajna Protocol - 0xTheC0der's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L170-L216 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L466-L485

Vulnerability details

Impact

The PositionManager.memorializePositions(params_) method can be called by anyone (per design, see 3rd party test cases) and allows insignificantly small (any value > 0) positions to be attached to anyone else's positions NFT, see PoC. As a result, the positionIndexes[params_.tokenId] storage array for an NFT with given token ID can be spammed with positions without the NFT owner's consent.

Therefore, the PositionManager.getPositionIndexesFiltered(tokenId_) method might exceed the block gas limit when iterating the positionIndexes[tokenId_] storage array. However, the RewardsManager.calculateRewards(...) and RewardsManager._calculateAndClaimRewards(...) methods rely on the aforementioned method to succeed in order to calculate and pay rewards.

All in all, a griefer can spam anyone's position NFT with insignificant positions until the rewards mechanism fails for the NFT owner due to DoS (gas limit). Side note: A position NFT also cannot be burned as long as such insignificant positions are attached to it, see PositionManager.burn(...).

Proof of Concept

The following diff is based on the existing test case testMemorializePositions in PositionManager.t.sol and demonstrates that insignificant positions can be attached by anyone.

diff --git a/ajna-core/tests/forge/unit/PositionManager.t.sol b/ajna-core/tests/forge/unit/PositionManager.t.sol
index bf3aa40..56c85d1 100644
--- a/ajna-core/tests/forge/unit/PositionManager.t.sol
+++ b/ajna-core/tests/forge/unit/PositionManager.t.sol
@@ -122,6 +122,7 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract
      */
     function testMemorializePositions() external {
         address testAddress = makeAddr("testAddress");
+        address otherAddress = makeAddr("otherAddress");
         uint256 mintAmount  = 10000 * 1e18;

         _mintQuoteAndApproveManagerTokens(testAddress, mintAmount);
@@ -134,17 +135,17 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract

         _addInitialLiquidity({
             from:   testAddress,
-            amount: 3_000 * 1e18,
+            amount: 1, //3_000 * 1e18,
             index:  indexes[0]
         });
         _addInitialLiquidity({
             from:   testAddress,
-            amount: 3_000 * 1e18,
+            amount: 1, //3_000 * 1e18,
             index:  indexes[1]
         });
         _addInitialLiquidity({
             from:   testAddress,
-            amount: 3_000 * 1e18,
+            amount: 1, // 3_000 * 1e18,
             index:  indexes[2]
         });

@@ -165,17 +166,20 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract

         // allow position manager to take ownership of the position
         uint256[] memory amounts = new uint256[](3);
-        amounts[0] = 3_000 * 1e18;
-        amounts[1] = 3_000 * 1e18;
-        amounts[2] = 3_000 * 1e18;
+        amounts[0] = 1; //3_000 * 1e18;
+        amounts[1] = 1; //3_000 * 1e18;
+        amounts[2] = 1; //3_000 * 1e18;
         _pool.increaseLPAllowance(address(_positionManager), indexes, amounts);

         // memorialize quote tokens into minted NFT
+        changePrank(otherAddress); // switch other address (not owner of NFT)
         vm.expectEmit(true, true, true, true);
-        emit TransferLP(testAddress, address(_positionManager), indexes, 9_000 * 1e18);
+        emit TransferLP(testAddress, address(_positionManager), indexes, 3 /*9_000 * 1e18*/);
         vm.expectEmit(true, true, true, true);
         emit MemorializePosition(testAddress, tokenId, indexes);
-        _positionManager.memorializePositions(memorializeParams);
+        _positionManager.memorializePositions(memorializeParams);  // switch back to test address (owner of NFT)
+        changePrank(testAddress);
+

         // check memorialization success
         uint256 positionAtPriceOneLP = _positionManager.getLP(tokenId, indexes[0]);

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Requiring that The PositionManager.memorializePositions(params_) can only be called by the NFT owner or anyone who has approval would help but break the 3rd party test cases.
Alternatively, one could enforce a minimum position value to make this griefing attack extremely unattractive.

Assessed type

DoS

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