PerpV2LeverageModule Audit Fixes

[cgewecke]

Internal Audit

• cf: Google Doc

Miscellaneous

• Abstract allowedSet logic into a separate contract that can be inherited ae35b81
• Move the following functions into a UniswapV3Math library 4d08b58
  • _formatSqrtPriceX96ToPriceX96
  • _formatX96ToX10_18
• Move the following function into a UnitConversion library aee5f51
  • _fromPreciseUnitToDecimals (uint256 version)
  • _fromPreciseUnitToDecimals (int version)
  • _toPreciseUnitsFromDecimals
• Move _abs to PreciseUnitMath e1d39dc

Structs

• ActionInfo: refactor isBaseToQuote, isExactInput to isBuy 229f2c4
• Line 76: Change name to baseTokenAmount and update comment accordingly 0693c55
• Line 77: Fix _createAndValidateActionInfoNotionalNotional b1555ca
• Line 95: Improve comment to clarify what positive and negative values mean a199cdd

Events

• Line 110: PerpTrade → PerpTraded, consistent in using the past tense d380e1b

Variables

• Line 198: Make positions internal 17fb7ca

Functions

Constructor

• Line 211: Update javadoc to note collateral token info is set here as well 2401ea2

trade

• Line 275: Improve _receiveQuoteQuantityUnits name. We use a version of "bound" elsewhere 3cad226
• Line 300: Instead of using equality lets use data from actionInfo since it already checks that equality d5d4cad

removeModule

• Line 400: We should be validating that there is < 1 positionUnit of USDC left since the max we can withdraw is outstandingCollateralUnits*totalSupply. There's a good chance that some dust amount (greater than 1 wei) of USDC will be left that we won't have access to. Let's add a comment that says what we are trying to check. 91e3d14

moduleIssueHook

• Skip if no external position for this module 3324053

moduleRedeemHook

• Skip if no external position for this module 3324053

getIssuanceAdjustments

• Javadoc: Note that the values in the returned arrays map to the same index in the components array. Note refactor plans. ecf05fb

getRedemptionAdjustments

• Javadoc: Note that the values in the returned arrays map to the same index in the components array. Note refactor plans. ecf05fb

getPositionNotionalInfo

• Put all instances of positions[_setToken][i] into a variable 5c6ba08

getPositionUnitInfo

• Put all instances of positions[_setToken][i] into a variable 5c6ba08
• Is it worth making an internal version that passes in totalSupply since we seem to make duplicate calls to totalSupply when we use this internally? Then expose an external end point that also queries totalSupply

_createAndValidateActionInfo

• In natspec let's note this is only called in trade bed2551
• Add validation that _baseToken is a valid token in the Perp system d943734

_createAndValidateActionInfoNotional

• Rename function since we don't do any validations here, _createActionInfoNotional b1555ca
• In natspec let's add where this function is called (issue and redeem hooks) b1555ca

_updatePositionList

• Handle baseBalance dust. Need to allow for less than 1 positionUnit of balance to be considered 0
• Line 1146 & 1148: Save positionList.contains(_baseToken) in variable and reuse 2596d7b

_executeModuleIssuanceHook

• Rename to _executePositionTrades 7320f20
• Refactor as Reference implementation for refactoring issue/redeem hooks. #176 with recommended changes 7320f20
• Line 826: divCeil to ensure over-collateralization 25c21fe

_executeModuleRedemptionHook

• Remove 7320f20

_getReducedOpenNotional

• Remove 7320f20

_formatX96ToX10_18

• Line 1256: Let's use our conventions here and do PreciseUnitMath.preciseUnit() instead of ether 1 4d08b58

Iosiro

5.1.1 Liquidations can prevent issuing and redeeming

• Sync positions before #_executePositionTrades: c85a0f1, 892ba14

5.1.2 Invalid state after liquidation

• Handle negative account balances after liquidation in #removeModule 4adb6fc
• Discuss w/Perp

5.2.1 Unexpected reverts when querying TWAP oracle

• Discuss w/ Perp

Missing Natspec

• L523: Add a description for the _isEquity parameter value. 34f16ac
• L553: Add a description for the _isEquity parameter value. 34f16ac
• L709: Add descriptions for the netQuoteBalance return value in the accountInfo struct. 7df0c57
• L904: Add descriptions for the _setToken and _collateralNotionalQuantity parameter values. 34f16ac
• L921: Add descriptions for the _setToken and _collateralQuantityUnits parameter values and the collateralNotionalQuantity return value. 34f16ac
• L955: Add descriptions for the _setToken and _collateralNotionalQuantity parameter values. 34f16ac
• L969: Add descriptions for _setToken and _collateralQuantityUnits parameter values and for collateralNotionalQuantity return value. 34f16ac
• L1003: Add a description for the _actionInfo parameter value. 34f16ac
• L1025: Add a description for the _actionInfo parameter value. 34f16ac
• L1042: Add descriptions for the _setToken and _exchangedQuantity parameter values. 34f16ac
• L1142: Add descriptions for the _setToken and _baseToken parameter values. 34f16ac
• L1157: Add descriptions for the _setTokenQuantity, _basePositionUnit and _positionInfo parameter values and the quoteBalance return value. 34f16ac
• L1176: Add descriptions for the _setToken parameter value and the return value. 34f16ac
• L1202: Add descriptions for the _setToken parameter value and the balance return value. 34f16ac
• L1215: Add descriptions for the _setToken, _components and _newExternalPositionUnit parameter values and the equityAdjustments and debtAdjustments return values. 34f16ac
• L1246: Add descriptions for the sqrtPriceX96 parameter value and the return value. 8015d03
• L1255: Add descriptions for the valueX96 parameter value and the return value. 8015d03
• L1266: Add descriptions for the amount and decimals parameter values and the return value. 5fbaa3d
• L1277: Add descriptions for the amount and decimals parameter values and the return value. 5fbaa3d
• L1286: Add descriptions for the amount and decimals parameter values and the return value. 5fbaa3d

Code layout

The code layout in the PerpV2LeverageModule could be improved through the following refactors:

• Move the initialize and other standard external manager functions to after the constructor, as the initializer will be the first function invoked after the constructor. 8c6b4bb
• Define an interface to simplify integration using Web3 clients. for
  • SlippageIssuanceModule (separate PR, e.g as part of planned changes there?)
  • DebtIssuanceModule (separate PR)
  • PerpV2LeverageModule 00a2bab
    • NOTE: Have not made PerpV2LeverageModule "implement" this interface. Iosiro suggested this addition as a convenience for Web3 consumers but if we have the leverage module use it, we would break some conventions about what files we define structs in etc. Probably something to discuss before going forward....

Spelling and grammar

• L53: simultaneuosly -> simultaneously 2d87b87
• L120 and PerpV2LeverageModule.sol#L132: redeeem -> redeem 2d87b87
• L190: setllement -> settlement 2d87b87

....

[bweick]

I'm struggling to come up with a better name right now but AllowSetToken feels like it doesn't convey everything we want. Does SetTokenAccessible sound better?

[bweick]

Small nit I just realized PerpV2LeverageModule

[cgewecke]

8f9aa56