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

[grandizzy]

Description of change

High level

• see branch_indigo - Lenders lose interests and pay deposit fees due to no slippage control sherlock-audit/2023-04-ajna-judging#72
  • new revertIfBelowLup param added to Pool.moveQuoteToken and PositionManager.moveLiquidity function
  • if true then the tx will revert if move happens below LUP price (hence avoid paying fee).
  • if false move below LUP will go through and deposit fee paid
  • MoveQuoteParams struct moved from inline in order to avoid stack too deep error
  • shrink Pool contract size by reading structs in view functions only once

Contract size

Pre Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,532B  (99.82%)
  ERC20Pool                -  23,815B  (96.90%)
  LenderActions            -  10,669B  (43.41%)

Post Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,567B  (99.96%)
  ERC20Pool                -  23,850B  (97.04%)
  LenderActions            -  10,715B  (43.60%)

Gas usage

Pre Change

| moveQuoteToken                       | 847             | 152897 | 106051 | 632900 | 39      |
| moveQuoteToken                         | 40229           | 45431  | 40397  | 65849    | 5       |

Post Change

| moveQuoteToken                       | 1918            | 144589 | 106768 | 634238 | 40      |
| moveQuoteToken                         | 41558           | 46707  | 41726  | 66912    | 5       |

[EdNoepel]

This is consistent with the proposed approach.

Assume the lender is already below the LUP, but is trying to move above the LUP. Even if they pass true, seems they will not get a revert. So the slippage protection is really only for those lenders currently above the LUP?

[grandizzy]

that's correct. lenders moving from below LUP won't pay deposit fee so slippage control to protect against is not needed

[EdNoepel]

Alright; makes sense.

[dmitriia]

Shouldn't revert list be expanded with revertIfBelowLup_ case:

https://github.com/ajna-finance/contracts/blob/1f9d10259db03a94318fb39d9dddcee74fce0cbd/src/PositionManager.sol#L267-L287

    /**
     *  @inheritdoc IPositionManagerOwnerActions
     *  @dev    External calls to `Pool` contract:
     *  @dev    `bucketInfo()`: get from bucket info
     *  @dev    `moveQuoteToken()`: move liquidity between buckets
     *  @dev    === Write state ===
     *  @dev    `TokenInfo.positionIndexes`: remove from bucket index
     *  @dev    `TokenInfo.positionIndexes`: add to bucket index
     *  @dev    `TokenInfo.positions`: update from bucket position
     *  @dev    `TokenInfo.positions`: update to bucket position
     *  @dev    === Revert on ===
     *  @dev    - `mayInteract`:
     *  @dev      token id is not a valid / minted id
     *  @dev      sender is not owner `NoAuth()`
     *  @dev      token id not minted for given pool `WrongPool()`
     *  @dev    - positions token to burn has liquidity `RemovePositionFailed()`
     *  @dev    - tried to move from bankrupt bucket `BucketBankrupt()`
     *  @dev    === Emit events ===
     *  @dev    - `MoveQuoteToken`
     *  @dev    - `MoveLiquidity`
     */

[grandizzy]

the possible underlying reverts are not listed here but only the reverts from function logic

[dmitriia]

Looks ok

[prateek105]

LGTM