Sherlock-72 (continuation - move quote token): Lenders pay deposit fe…

…es due to no slippage control (#918)

* Sherlock-72 (continuation - move quote token): Lenders pay deposit fees due to no slippage control

* Shrink Pool contract size, fix brownie test

src/PositionManager.sol

@@ -290,7 +290,8 @@ contract PositionManager is PermitERC721, IPositionManager, Multicall, Reentranc
  uint256 tokenId_,
  uint256 fromIndex_,
  uint256 toIndex_,
- uint256 expiry_
+ uint256 expiry_,
+ bool revertIfBelowLup_
  ) external override nonReentrant mayInteract(pool_, tokenId_) {
  TokenInfo storage tokenInfo = positionTokens[tokenId_];
  Position storage fromPosition = tokenInfo.positions[fromIndex_];
@@ -334,7 +335,8 @@ contract PositionManager is PermitERC721, IPositionManager, Multicall, Reentranc
  vars.maxQuote,
  fromIndex_,
  toIndex_,
- expiry_
+ expiry_,
+ revertIfBelowLup_
  );
 
  EnumerableSet.UintSet storage positionIndexes = tokenInfo.positionIndexes;

src/base/Pool.sol

@@ -25,13 +25,15 @@ import {
  PoolState,
  AuctionsState,
  DepositsState,
+ Loan,
  LoansState,
  InflatorState,
  EmaState,
  InterestState,
  PoolBalancesState,
  ReserveAuctionState,
  Bucket,
+ Lender,
  BurnEvent,
  Liquidation
 } from '../interfaces/pool/commons/IPoolState.sol';
@@ -182,13 +184,21 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  uint256 maxAmount_,
  uint256 fromIndex_,
  uint256 toIndex_,
- uint256 expiry_
+ uint256 expiry_,
+ bool revertIfBelowLup_
  ) external override nonReentrant returns (uint256 fromBucketLP_, uint256 toBucketLP_, uint256 movedAmount_) {
  _revertAfterExpiry(expiry_);
  PoolState memory poolState = _accruePoolInterest();
 
  _revertIfAuctionDebtLocked(deposits, poolState.t0DebtInAuction, fromIndex_, poolState.inflator);
 
+ MoveQuoteParams memory moveParams;
+ moveParams.maxAmountToMove = maxAmount_;
+ moveParams.fromIndex = fromIndex_;
+ moveParams.toIndex = toIndex_;
+ moveParams.thresholdPrice = Loans.getMax(loans).thresholdPrice;
+ moveParams.revertIfBelowLup = revertIfBelowLup_;
+
  uint256 newLup;
  (
  fromBucketLP_,
@@ -199,12 +209,7 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  buckets,
  deposits,
  poolState,
- MoveQuoteParams({
- maxAmountToMove: maxAmount_,
- fromIndex: fromIndex_,
- toIndex: toIndex_,
- thresholdPrice: Loans.getMax(loans).thresholdPrice
- })
+ moveParams
  );
 
  // update pool interest rate state
@@ -785,10 +790,11 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  uint256 index_
  ) external view override returns (uint256, uint256, uint256, uint256, uint256) {
  uint256 scale = Deposits.scale(deposits, index_);
+ Bucket storage bucket = buckets[index_];
  return (
- buckets[index_].lps,
- buckets[index_].collateral,
- buckets[index_].bankruptcyTime,
+ bucket.lps,
+ bucket.collateral,
+ bucket.bankruptcyTime,
  Maths.wmul(scale, Deposits.unscaledValueAt(deposits, index_)),
  scale
  );
@@ -826,15 +832,20 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
 
  /// @inheritdoc IPoolState
  function debtInfo() external view returns (uint256, uint256, uint256, uint256) {
- uint256 pendingInflator = PoolCommons.pendingInflator(
- inflatorState.inflator,
- inflatorState.inflatorUpdate,
- interestState.interestRate
- );
+ uint256 t0Debt = poolBalances.t0Debt;
+ uint256 inflator = inflatorState.inflator;
+
  return (
- Maths.ceilWmul(poolBalances.t0Debt, pendingInflator),
- Maths.ceilWmul(poolBalances.t0Debt, inflatorState.inflator),
- Maths.ceilWmul(poolBalances.t0DebtInAuction, inflatorState.inflator),
+ Maths.ceilWmul(
+ t0Debt,
+ PoolCommons.pendingInflator(
+ inflator,
+ inflatorState.inflatorUpdate,
+ interestState.interestRate
+ )
+ ),
+ Maths.ceilWmul(t0Debt, inflator),
+ Maths.ceilWmul(poolBalances.t0DebtInAuction, inflator),
  interestState.t0Debt2ToCollateral
  );
  }
@@ -906,8 +917,11 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  uint256 index_,
  address lender_
  ) external view override returns (uint256 lpBalance_, uint256 depositTime_) {
- depositTime_ = buckets[index_].lenders[lender_].depositTime;
- if (buckets[index_].bankruptcyTime < depositTime_) lpBalance_ = buckets[index_].lenders[lender_].lps;
+ Bucket storage bucket = buckets[index_];
+ Lender storage lender = bucket.lenders[lender_];
+
+ depositTime_ = lender.depositTime;
+ if (bucket.bankruptcyTime < depositTime_) lpBalance_ = lender.lps;
  }
 
  /// @inheritdoc IPoolState
@@ -923,17 +937,19 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  function loanInfo(
  uint256 loanId_
  ) external view override returns (address, uint256) {
+ Loan memory loan = Loans.getByIndex(loans, loanId_);
  return (
- Loans.getByIndex(loans, loanId_).borrower,
- Loans.getByIndex(loans, loanId_).thresholdPrice
+ loan.borrower,
+ loan.thresholdPrice
  );
  }
 
  /// @inheritdoc IPoolState
  function loansInfo() external view override returns (address, uint256, uint256) {
+ Loan memory maxLoan = Loans.getMax(loans);
  return (
- Loans.getMax(loans).borrower,
- Maths.wmul(Loans.getMax(loans).thresholdPrice, inflatorState.inflator),
+ maxLoan.borrower,
+ Maths.wmul(maxLoan.thresholdPrice, inflatorState.inflator),
  Loans.noOfLoans(loans)
  );
  }

src/interfaces/pool/commons/IPoolErrors.sol

@@ -180,7 +180,7 @@ interface IPoolErrors {
  error PoolUnderCollateralized();
 
  /**
- * @notice Actor is attempting to add quote tokens at a price below the `LUP`.
+ * @notice Actor is attempting to add or move quote tokens at a price below the `LUP`.
  * @notice Actor is attempting to kick with bucket price below the `LUP`.
  */
  error PriceBelowLUP();

src/interfaces/pool/commons/IPoolInternals.sol

@@ -78,10 +78,11 @@ struct AddQuoteParams {
 
 /// @dev Struct used to hold parameters for `LenderAction.moveQuoteToken` action.
 struct MoveQuoteParams {
- uint256 fromIndex; // the deposit index from where amount is moved
- uint256 maxAmountToMove; // [WAD] max amount to move between deposits
- uint256 toIndex; // the deposit index where amount is moved to
- uint256 thresholdPrice; // [WAD] max threshold price in pool
+ uint256 fromIndex; // the deposit index from where amount is moved
+ uint256 maxAmountToMove; // [WAD] max amount to move between deposits
+ uint256 toIndex; // the deposit index where amount is moved to
+ uint256 thresholdPrice; // [WAD] max threshold price in pool
+ bool revertIfBelowLup; // revert tx if quote token is moved from above the LUP to below the LUP
 }
 
 /// @dev Struct used to hold parameters for `LenderAction.removeQuoteToken` action.

src/interfaces/pool/commons/IPoolLenderActions.sol

@@ -28,19 +28,21 @@ interface IPoolLenderActions {
 
  /**
  * @notice Called by lenders to move an amount of credit from a specified price bucket to another specified price bucket.
- * @param maxAmount_ The maximum amount of quote token to be moved by a lender (`WAD` precision).
- * @param fromIndex_ The bucket index from which the quote tokens will be removed.
- * @param toIndex_ The bucket index to which the quote tokens will be added.
- * @param expiry_ Timestamp after which this transaction will revert, preventing inclusion in a block with unfavorable price.
- * @return fromBucketLP_ The amount of `LP` moved out from bucket (`WAD` precision).
- * @return toBucketLP_ The amount of `LP` moved to destination bucket (`WAD` precision).
- * @return movedAmount_ The amount of quote token moved (`WAD` precision).
+ * @param maxAmount_ The maximum amount of quote token to be moved by a lender (`WAD` precision).
+ * @param fromIndex_ The bucket index from which the quote tokens will be removed.
+ * @param toIndex_ The bucket index to which the quote tokens will be added.
+ * @param expiry_ Timestamp after which this transaction will revert, preventing inclusion in a block with unfavorable price.
+ * @param revertIfBelowLup_ The tx will revert if quote token is moved from above the `LUP` to below the `LUP` (and avoid paying fee for move below `LUP`).
+ * @return fromBucketLP_ The amount of `LP` moved out from bucket (`WAD` precision).
+ * @return toBucketLP_ The amount of `LP` moved to destination bucket (`WAD` precision).
+ * @return movedAmount_ The amount of quote token moved (`WAD` precision).
  */
  function moveQuoteToken(
  uint256 maxAmount_,
  uint256 fromIndex_,
  uint256 toIndex_,
- uint256 expiry_
+ uint256 expiry_,
+ bool revertIfBelowLup_
  ) external returns (uint256 fromBucketLP_, uint256 toBucketLP_, uint256 movedAmount_);
 
  /**

src/interfaces/position/IPositionManagerOwnerActions.sol

@@ -50,18 +50,20 @@ interface IPositionManagerOwnerActions {
 
  /**
  * @notice Called by owners to move liquidity between two buckets.
- * @param pool_ The pool address associated with positions NFT.
- * @param tokenId_ The tokenId of the positions NFT.
- * @param fromIndex_ The bucket index from which liquidity should be moved.
- * @param toIndex_ The bucket index to which liquidity should be moved.
- * @param expiry_ Timestamp after which this TX will revert, preventing inclusion in a block with unfavorable price.
+ * @param pool_ The pool address associated with positions NFT.
+ * @param tokenId_ The tokenId of the positions NFT.
+ * @param fromIndex_ The bucket index from which liquidity should be moved.
+ * @param toIndex_ The bucket index to which liquidity should be moved.
+ * @param expiry_ Timestamp after which this TX will revert, preventing inclusion in a block with unfavorable price.
+ * @param revertIfBelowLup_ The tx will revert if quote token is moved from above the `LUP` to below the `LUP` (and avoid paying fee for move below `LUP`).
  */
  function moveLiquidity(
  address pool_,
  uint256 tokenId_,
  uint256 fromIndex_,
  uint256 toIndex_,
- uint256 expiry_
+ uint256 expiry_,
+ bool revertIfBelowLup_
  ) external;
 
  /**

src/libraries/external/LenderActions.sol

@@ -142,6 +142,7 @@ library LenderActions {
  * @dev same block when bucket becomes insolvent `BucketBankruptcyBlock()`
  * @dev no LP awarded in bucket `InsufficientLP()`
  * @dev calculated unscaled amount to add is 0 `InvalidAmount()`
+ * @dev deposit below `LUP` `PriceBelowLUP()`
  * @dev === Emit events ===
  * @dev - `AddQuoteToken`
  */
@@ -233,6 +234,7 @@ library LenderActions {
  * @dev dust amount `DustAmountNotExceeded()`
  * @dev invalid index `InvalidIndex()`
  * @dev no LP awarded in to bucket `InsufficientLP()`
+ * @dev move below `LUP` `PriceBelowLUP()`
  * @dev === Emit events ===
  * @dev - `BucketBankruptcy`
  * @dev - `MoveQuoteToken`
@@ -288,6 +290,8 @@ library LenderActions {
  lup_ = Deposits.getLup(deposits_, poolState_.debt);
  // apply unutilized deposit fee if quote token is moved from above the LUP to below the LUP
  if (vars.fromBucketPrice >= lup_ && vars.toBucketPrice < lup_) {
+ if (params_.revertIfBelowLup) revert PriceBelowLUP();
+
  movedAmount_ = Maths.wmul(movedAmount_, Maths.WAD - _depositFeeRate(poolState_.rate));
  }
 

tests/brownie/test_scaled_pool.py

@@ -25,7 +25,7 @@ def test_quote_deposit_move_remove_scaled(
 
  move_txes = []
  for i in range(2530, 2550):
- tx = scaled_pool.moveQuoteToken(100 * 10**18, i, i + 30, chain.time() + 30, {"from": lenders[0]})
+ tx = scaled_pool.moveQuoteToken(100 * 10**18, i, i + 30, chain.time() + 30, False, {"from": lenders[0]})
  move_txes.append(tx)
  with capsys.disabled():
  print("\n==================================")

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedERC20PoolPositionsHandler.sol

@@ -242,7 +242,7 @@ abstract contract UnboundedERC20PoolPositionsHandler is UnboundedBasePositionHan
  * @notice Struct holding parameters for moving the liquidity of a position.
  */
 
- try _positionManager.moveLiquidity(address(_pool), tokenId_, fromIndex_, toIndex_, block.timestamp + 30) {
+ try _positionManager.moveLiquidity(address(_pool), tokenId_, fromIndex_, toIndex_, block.timestamp + 30, false) {
 
  bucketIndexesByTokenId[tokenId_].add(toIndex_);
  bucketIndexesByTokenId[tokenId_].remove(fromIndex_);

tests/forge/invariants/PositionsAndRewards/handlers/unbounded/UnboundedERC721PoolPositionsHandler.sol

@@ -242,7 +242,7 @@ abstract contract UnboundedERC721PoolPositionsHandler is UnboundedBasePositionHa
  * @notice Struct holding parameters for moving the liquidity of a position.
  */
 
- try _positionManager.moveLiquidity(address(_pool), tokenId_, fromIndex_, toIndex_, block.timestamp + 30) {
+ try _positionManager.moveLiquidity(address(_pool), tokenId_, fromIndex_, toIndex_, block.timestamp + 30, false) {
 
  bucketIndexesByTokenId[tokenId_].add(toIndex_);
  bucketIndexesByTokenId[tokenId_].remove(fromIndex_);

tests/forge/invariants/base/handlers/unbounded/UnboundedBasicPoolHandler.sol

@@ -112,7 +112,8 @@ abstract contract UnboundedBasicPoolHandler is BaseHandler {
  amount_,
  fromIndex_,
  toIndex_,
- block.timestamp + 1 minutes
+ block.timestamp + 1 minutes,
+ false
  ) returns (uint256, uint256, uint256 movedAmount_) {
 
  (, uint256 fromBucketDepositTime) = _pool.lenderInfo(fromIndex_, _actor);

tests/forge/unit/ERC20Pool/ERC20DSTestPlus.sol

@@ -740,7 +740,7 @@ abstract contract ERC20DSTestPlus is DSTestPlus, IERC20PoolEvents {
  ) internal {
  changePrank(from);
  vm.expectRevert(IPoolErrors.LUPBelowHTP.selector);
- ERC20Pool(address(_pool)).moveQuoteToken(amount, fromIndex, toIndex, type(uint256).max);
+ ERC20Pool(address(_pool)).moveQuoteToken(amount, fromIndex, toIndex, type(uint256).max, false);
  }
 
 }

tests/forge/unit/ERC20Pool/ERC20PoolGasLoadTest.t.sol

@@ -278,7 +278,7 @@ contract ERC20PoolCommonActionsGasLoadTest is ERC20PoolGasLoadTest {
  _pool.removeQuoteToken(5_000 * 1e18, index_);
 
  skip(15 hours);
- _pool.moveQuoteToken(1_000 * 1e18, index_, index_ + 1, block.timestamp + 2 minutes);
+ _pool.moveQuoteToken(1_000 * 1e18, index_, index_ + 1, block.timestamp + 2 minutes, false);
 
  skip(15 hours);
  _pool.removeQuoteToken(type(uint256).max, index_);

tests/forge/unit/ERC20Pool/ERC20PoolInputValidation.t.sol

@@ -25,16 +25,16 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {
  function testValidateMoveQuoteTokenInput() external tearDown {
  // revert on zero amount
  vm.expectRevert(IPoolErrors.InvalidAmount.selector);
- _pool.moveQuoteToken(0, 1, 2, block.timestamp + 1);
+ _pool.moveQuoteToken(0, 1, 2, block.timestamp + 1, false);
  // revert on move to same index
  vm.expectRevert(IPoolErrors.MoveToSameIndex.selector);
- _pool.moveQuoteToken(1000, 1, 1, block.timestamp + 1);
+ _pool.moveQuoteToken(1000, 1, 1, block.timestamp + 1, false);
  // revert on to zero index
  vm.expectRevert(IPoolErrors.InvalidIndex.selector);
- _pool.moveQuoteToken(1000, 1, 0, block.timestamp + 1);
+ _pool.moveQuoteToken(1000, 1, 0, block.timestamp + 1, false);
  // revert on to index greater than max index
  vm.expectRevert(IPoolErrors.InvalidIndex.selector);
- _pool.moveQuoteToken(1000, 1, MAX_FENWICK_INDEX + 1, block.timestamp + 1);
+ _pool.moveQuoteToken(1000, 1, MAX_FENWICK_INDEX + 1, block.timestamp + 1, false);
  }
 
  function testValidateRemoveQuoteTokenInput() external tearDown {

tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsSettle.t.sol

@@ -812,7 +812,7 @@ contract ERC20PoolLiquidationsSettleTest is ERC20HelperContract {
  skip(1 hours);
 
  // move quote token in a bankrupt bucket should set deposit time to time of bankruptcy + 1 to prevent losing deposit
- _pool.moveQuoteToken(10 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes);
+ _pool.moveQuoteToken(10 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes, false);
  (, , uint256 bankruptcyTime, , ) = _pool.bucketInfo(_i9_91);
  _assertLenderLpBalance({
  lender: _lender1,
@@ -848,7 +848,7 @@ contract ERC20PoolLiquidationsSettleTest is ERC20HelperContract {
  depositTime: _startTime
  });
 
- _pool.moveQuoteToken(1_000 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes);
+ _pool.moveQuoteToken(1_000 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes, false);
 
  _assertLenderLpBalance({
  lender: _lender,
@@ -858,7 +858,7 @@ contract ERC20PoolLiquidationsSettleTest is ERC20HelperContract {
  });
 
  _pool.addQuoteToken(1_000 * 1e18, _i9_52, block.timestamp + 1 minutes, false);
- _pool.moveQuoteToken(1_000 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes);
+ _pool.moveQuoteToken(1_000 * 1e18, _i9_52, _i9_91, block.timestamp + 1 minutes, false);
 
  _assertLenderLpBalance({
  lender: _lender,
@@ -876,7 +876,7 @@ contract ERC20PoolLiquidationsSettleTest is ERC20HelperContract {
  exchangeRate: 0.821534360361016385 * 1e18
  });
 
- _pool.moveQuoteToken(10000000000 * 1e18, _i9_72, _i9_91, type(uint256).max);
+ _pool.moveQuoteToken(10000000000 * 1e18, _i9_72, _i9_91, type(uint256).max, false);
 
  _assertBucket({
  index: _i9_72,

tests/forge/unit/ERC20Pool/ERC20PoolQuoteToken.t.sol

@@ -1223,6 +1223,14 @@ contract ERC20PoolQuoteTokenTest is ERC20HelperContract {
  expiry: block.timestamp - 20
  });
 
+ // should revert if move below LUP with revertIfBelowLup set to true
+ _assertMoveDepositBelowLUPRevert({
+ from: _lender,
+ amount: 10_000 * 1e18,
+ fromIndex: 4549,
+ toIndex: 5000
+ });
+
  // should be charged unutilized deposit fee if moving below LUP
  _moveLiquidityWithPenalty({
  from: _lender,