sherlock-admin commented 1 year ago

osmanozdemir1

high

`removeCollateral` might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds

Summary

Bucket bankruptcy is not declared even though the bucket has zeroed out in the removeCollateral, and bankruptcy time is not updated. This might cause some transactions not to revert when they should revert. It might also cause previous depositors to gain an advantage, and new depositors to lose some of their LP shares when the bucket becomes solvent again.

Vulnerability Detail

Bankruptcy logic takes an important role in this protocol and it helps to get info about buckets. The bankruptcy time of a bucket is widely used throughout the protocol, especially when calculating the LPs. It is also used to prevent adding/moving collateral or quote token in the same block when the bucket becomes insolvent.

If a bucket doesn't have collateral and deposit but has LPs after a transaction, it is considered insolvent and the remaining LPs are forfeited. As a result of this bucket.lps will be assigned to zero and bucket.bankruptcyTime will be updated. At this stage, all three of the bucketCollateral, bucketDeposit and bucketLP will be zeroed out in that bucket, and bucket.bankruptcyTime will be the block.timestamp. But, this is not the case in the removeCollateral method if everything is zeroed out.

The removeCollateral function in the LenderActions.sol library has a logic that checks if the bucket collateral is going to be cleared or not.

// File: LenderActions.sol

504.  // If clearing out the bucket collateral, ensure it's zeroed out
505.  if (bucketLP == 0 && bucketDeposit == 0) {
506.       amount_ = bucketCollateral;
507.  }

509.  bucketCollateral  -= amount_;
510.  bucket.collateral = bucketCollateral;

If this check is true, amount_ will be assigned bucketCollateral. This will result bucketCollateral -= amount_ to be equal to 0, and bucket.collateral will also be 0 in lines 509 and 510.

Right after that the bankruptcy check will be performed in line 513 and below.

// File: LenderActions.sol   
         // check if bucket healthy after collateral remove - set bankruptcy if collateral and deposit are 0 but there's still LP
513.     if (bucketCollateral == 0 && bucketDeposit == 0 && bucketLP != 0) {
514.         bucket.lps            = 0;
515.         bucket.bankruptcyTime = block.timestamp;
516.
517.         emit BucketBankruptcy(
518.             index_,
519.             bucketLP
520.         );
521.     } else {
522.         // update lender and bucket LP balances
523.         lender.lps -= lpAmount_;
524.         bucket.lps = bucketLP;
525.     }

Because everything is zeroed out and bucketLP == 0, this check will fail and bucket bankruptcy time will not be updated. The bucket is insolvent, and the current stage of the bucket is exactly the same as a bankrupt bucket with one exception, bucket.bankruptyTime.

Impact

Not updating bucket bankruptcy time in a zeroed-out bucket might cause multiple issues.
First of all, addQuoteToken and moveQuoteToken methods in the LenderActions.sol library, and addCollateral method in the Buckets.sol library should revert if they are called in the same block when the bucket becomes insolvent. These methods will not revert and users might interact with a bucket just at the same time when the bucket becomes insolvent.

More importantly, bucket bankruptcy time is used widely in this protocol to calculate the LPs of each user. The LPs gained from the deposits before the bankruptcy are considered forfeited and start from zero for future deposits. But because the bankruptcy time is not updated, users might gain an advantage from the previous deposits to a bankrupt bucket. This will cause new depositors to lose some portion of their LPs when the bucket becomes solvent again.

For example, the latter can happen with the moveLiquidity method in the PositionManager.sol contract. Users can move their liquidity from one bucket index to another one. Someone can increase their LPs even if the bucket becomes insolvent after their deposit.

//File: PositionManager.sol    
373.    // reset LP in TO memorialized position if bucket went bankrupt after memorialization
374.    if (_bucketBankruptAfterDeposit(IPool(pool_), toIndex_, vars.toDepositTime)) {
375.        toPosition.lps = vars.lpbAmountTo;
376.    } else {
377.        toPosition.lps += vars.lpbAmountTo;
378.    }

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L504-L510

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L512-L525

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/PositionManager.sol#L373-L378

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/PositionManager.sol#L518-L526

Tool used

Manual Review

Recommendation

I would recommend declaring bucket bankruptcy if the collateral and deposit are 0, without checking if the bucket has LPs left or not.

osmanozdemir1 commented 1 year ago

Escalate for 10 USDC

I don't agree that this issue is considered as a duplicate of #100, and not validated for reward.

I understand and acknowledge that loss of unclaimed rewards in a bankrupt bucket is the protocol's design, and that is not the issue I submitted here.

The issue I submitted is that a bucket is not considered as bankrupted even though it is actually bankrupted. As a result of this, the bucket bankruptcy time is not updated, and this will cause unexpected behaviour as some of the functions in the protocol should not be executed in the same block with a bankruptcy. These functions are addQuoteToken, moveQuoteToken and addCollateral, which are major functions of the protocol.

The reason bucket bankruptcy is not emitted and bankruptcy time is not updated is this: https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/libraries/external/LenderActions.sol#L504-L513

// File: LenderActions.sol

504.  // If clearing out the bucket collateral, ensure it's zeroed out
505.  if (bucketLP == 0 && bucketDeposit == 0) {
506.       amount_ = bucketCollateral;
507.  }

509.  bucketCollateral  -= amount_;
510.  bucket.collateral = bucketCollateral;
511. 
512.          // check if bucket healthy after collateral remove - set bankruptcy if collateral and deposit are 0 but there's still LP
513.     if (bucketCollateral == 0 && bucketDeposit == 0 && bucketLP != 0) {

If the check in line 505 is true, amount_ will be assigned bucketCollateral. This will result bucketCollateral -= amount_ to be equal to 0, and bucket.collateral will also be 0 in lines 509 and 510.

Because of bucketCollateral == 0, bucketDeposit == 0, and also bucketLP == 0, the check in line 513 will fail, the bucket will not be considered as bankrupt. Bankruptcy time will not be updated even though it is completely zeroed out.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

I don't agree that this issue is considered as a duplicate of #100, and not validated for reward.

I understand and acknowledge that loss of unclaimed rewards in a bankrupt bucket is the protocol's design, and that is not the issue I submitted here.

The issue I submitted is that a bucket is not considered as bankrupted even though it is actually bankrupted. As a result of this, the bucket bankruptcy time is not updated, and this will cause unexpected behaviour as some of the functions in the protocol should not be executed in the same block with a bankruptcy. These functions are addQuoteToken, moveQuoteToken and addCollateral, which are major functions of the protocol.

The reason bucket bankruptcy is not emitted and bankruptcy time is not updated is this: https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/libraries/external/LenderActions.sol#L504-L513
// File: LenderActions.sol

504.  // If clearing out the bucket collateral, ensure it's zeroed out
505.  if (bucketLP == 0 && bucketDeposit == 0) {
506.       amount_ = bucketCollateral;
507.  }

509.  bucketCollateral  -= amount_;
510.  bucket.collateral = bucketCollateral;
511. 
512.          // check if bucket healthy after collateral remove - set bankruptcy if collateral and deposit are 0 but there's still LP
513.     if (bucketCollateral == 0 && bucketDeposit == 0 && bucketLP != 0) {
If the check in line 505 is true, amount_ will be assigned bucketCollateral. This will result bucketCollateral -= amount_ to be equal to 0, and bucket.collateral will also be 0 in lines 509 and 510.

Because of bucketCollateral == 0, bucketDeposit == 0, and also bucketLP == 0, the check in line 513 will fail, the bucket will not be considered as bankrupt. Bankruptcy time will not be updated even though it is completely zeroed out.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

dmitriia commented 1 year ago

How can this impact happen if LPs are completely zeroed out?

More importantly, bucket bankruptcy time is used widely in this protocol to calculate the LPs of each user. The LPs gained from the deposits before the bankruptcy are considered forfeited and start from zero for future deposits. But because the bankruptcy time is not updated, users might gain an advantage from the previous deposits to a bankrupt bucket. This will cause new depositors to lose some portion of their LPs when the bucket becomes solvent again.

For example, the latter can happen with the moveLiquidity method in the PositionManager.sol contract. Users can move their liquidity from one bucket index to another one. Someone can increase their LPs even if the bucket becomes insolvent after their deposit.

If user deposits into such zeroed bucket in the same block after zeroing it will cause no issues (they will own what they deposited), and they cannot withdraw anything as LPs are zeroed:

First of all, addQuoteToken and moveQuoteToken methods in the LenderActions.sol library, and addCollateral method in the Buckets.sol library should revert if they are called in the same block when the bucket becomes insolvent. These methods will not revert and users might interact with a bucket just at the same time when the bucket becomes insolvent.

It looks like there is no impact demonstrated. The bucketCollateral == 0 && bucketDeposit == 0 && bucketLP != 0 logic is reiterated many times across the protocol, as exactly non-zero LPs left are the reason for this bankruptcy mechanics to exist.

osmanozdemir1 commented 1 year ago

I think the PositionManager contract can cause the impact above. https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/PositionManager.sol#L373-L378

        // reset LP in TO memorialized position if bucket went bankrupt after memorialization
        if (_bucketBankruptAfterDeposit(IPool(pool_), toIndex_, vars.toDepositTime)) {
            toPosition.lps = vars.lpbAmountTo;
        } else {
            toPosition.lps += vars.lpbAmountTo;
        }

Users can move liquidity in the position manager contract for their corresponding token ID. This will first call moveQuoteToken to move the liquidity and then update the position for that indexes. If the bankruptcy is not emitted, user can still increase their previous position LP counts instead of starting from 0.

Bob creates a position adding liquidity to 10 buckets.
Moves first 4 of them to the 5th bucket using PositionManager contract with his tokenID. His bucket LPs will be moved to that bucket and also his toPosition.lps will increase for the 5th bucket.
Then 5th bucket goes bankrupt as above and everything zeroed out, but bankruptcy time is not updated.
Bob moves other 5 bucket's (6 to 10) liquidity to the 5th bucket too after this event.
After the bankruptcy, his position LPs should start from 0, but because of bankruptcy is not emitted, he will still get the LP's from previous deposits.
Then he redeems his position. He didn't suffer from the bankruptcy but he should have lost some of his LP's according to business logic of the protocol.

The bucket zeroed out but he didn't lose his position LP's in the PositionManager contract.

grandizzy commented 1 year ago

I think the PositionManager contract can cause the impact above. https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/PositionManager.sol#L373-L378
        // reset LP in TO memorialized position if bucket went bankrupt after memorialization
        if (_bucketBankruptAfterDeposit(IPool(pool_), toIndex_, vars.toDepositTime)) {
            toPosition.lps = vars.lpbAmountTo;
        } else {
            toPosition.lps += vars.lpbAmountTo;
        }
Users can move liquidity in the position manager contract for their corresponding token ID. This will first call moveQuoteToken to move the liquidity and then update the position for that indexes. If the bankruptcy is not emitted, user can still increase their previous position LP counts instead of starting from 0.
* Bob creates a position adding liquidity to 10 buckets.

* Moves first 4 of them to the 5th bucket using PositionManager contract with his tokenID. His bucket LPs will be moved to that bucket and also his `toPosition.lps` will increase for the 5th bucket.

* Then 5th bucket goes bankrupt as above and everything zeroed out, but bankruptcy time is not updated.

* Bob moves other 5 bucket's (6 to 10) liquidity to the 5th bucket too after this event.

* After the bankruptcy, his position LPs should start from 0, but because of bankruptcy is not emitted, he will still get the LP's from previous deposits.

* Then he redeems his position. He didn't suffer from the bankruptcy but he should have lost some of his LP's according to business logic of the protocol.
The bucket zeroed out but he didn't lose his position LP's in the PositionManager contract.

PositionManager.moveLiquidity deals only with quote token (and not collateral) by calling moveQuoteToken which takes into account bucket bankruptcy https://github.com/ajna-finance/ajna-core/blob/main/src/libraries/external/LenderActions.sol#L317

hrishibhat commented 1 year ago

@osmanozdemir1 based on sponsor's comment this seems to be a non-issue. Do you have any further comments?

osmanozdemir1 commented 1 year ago

@osmanozdemir1 based on sponsor's comment this seems to be a non-issue. Do you have any further comments?

No, I respect the results.

hrishibhat commented 1 year ago

Result: Invalid Unique Considering this a non-issue based on Sposnor comments

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

osmanozdemir1: accepted

sherlock-audit / 2023-04-ajna-judging

osmanozdemir1 - `removeCollateral` might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds #97

`removeCollateral` might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2023-04-ajna-judging

osmanozdemir1 - `removeCollateral` might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds #97

removeCollateral might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

`removeCollateral` might cause a bucket to bankrupt but not update bankruptcy time, which might cause unexpected behavior and loss of funds