fix(PerpV2BasisTradingModule): External audit fixes for low-risk findings and Internal audit fixes (#232)

* Design Improvement: Cache storage slot values

* Design Improvements: Spelling and grammar

* Fix typo

* Fix revert statement

* Design improvement: Return early if no funding will be withdrawn

* Fix coverage: Add test case for when notionalFunding is zero

* Fix failing test

* Modify calculateExternalPositionUnit logic used by deposit and withdraw; Reduce bytecodesize

* Override withdraw function to track settled funding

* Fix deposit; Add check to ensure deposit unit is <= current unit; Add tests

* Refactor update external position unit logic; Add tests

* Fix tests; And skip 2 getRedemptionAdjustments tests

* Add internal audit fixes and suggestions

* Fix division by zero error

* fix(tests): Small fixes for BasisTradingModule audit changes (#243)

* Add fix for _updateExternalPositionUnit

* Add test for negative balance check in _calculateNetFeesPositionUnit

* Fix externalPositionUnit test in perpV2LeverageV2SlippageIssuance tests

* Use `tradeAndTrackFunding` everywhere in BasisTrading integration tests

* Remove .skip; Fix javadocs

Co-authored-by: cgewecke <[email protected]>

Author: Sachin

contracts/interfaces/external/perp-v2/IAccountBalance.sol

@@ -28,8 +28,8 @@ interface IAccountBalance {
     function getBase(address trader, address baseToken) external view returns (int256);
     function getQuote(address trader, address baseToken) external view returns (int256);
     function getNetQuoteBalanceAndPendingFee(address trader) external view returns (int256, uint256);
-    function getPositionSize(address trader, address baseToken) external view returns (int256);
-    function getPositionValue(address trader, address baseToken) external view returns (int256);
+    function getTotalPositionSize(address trader, address baseToken) external view returns (int256);
+    function getTotalPositionValue(address trader, address baseToken) external view returns (int256);
     function getTotalAbsPositionValue(address trader) external view returns (uint256);
     function getClearingHouseConfig() external view returns (address);
     function getExchange() external view returns (address);

contracts/protocol/lib/PositionV2.sol

@@ -37,7 +37,7 @@ import { PreciseUnitMath } from "../../lib/PreciseUnitMath.sol";
  * CHANGELOG:
  * - `Position` library has all internal functions which are inlined to the module contract during compilation.
  * Inlining functions increases bytecode size of the module contract. This library contains the same functions
- * as `Position` library but all the functions have public/external access modifier. Thus, making this version 
+ * as `Position` library but all the functions have public/external access modifier. Thus, making this version
  * linkable which helps in reducing bytecode size of the module contract.
  */
 library PositionV2 {
@@ -62,7 +62,7 @@ library PositionV2 {
     function hasExternalPosition(ISetToken _setToken, address _component) public view returns(bool) {
         return _setToken.getExternalPositionModules(_component).length > 0;
     }
-    
+
     /**
      * Returns whether the SetToken component default position real unit is greater than or equal to units passed in.
      */
@@ -71,7 +71,7 @@ library PositionV2 {
     }
 
     /**
-     * Returns whether the SetToken component external position is greater than or equal to the real units passed in.
+     * Returns whether the SetToken component's external position is greater than or equal to the real units passed in.
      */
     function hasSufficientExternalUnits(
         ISetToken _setToken,
@@ -83,12 +83,12 @@ library PositionV2 {
         view
         returns(bool)
     {
-       return _setToken.getExternalPositionRealUnit(_component, _positionModule) >= _unit.toInt256();    
+       return _setToken.getExternalPositionRealUnit(_component, _positionModule) >= _unit.toInt256();
     }
 
     /**
      * If the position does not exist, create a new Position and add to the SetToken. If it already exists,
-     * then set the position units. If the new units is 0, remove the position. Handles adding/removing of 
+     * then set the position units. If the new units is 0, remove the position. Handles adding/removing of
      * components where needed (in light of potential external positions).
      *
      * @param _setToken           Address of SetToken being modified
@@ -114,7 +114,7 @@ library PositionV2 {
 
     /**
      * Update an external position and remove and external positions or components if necessary. The logic flows as follows:
-     * 1) If component is not already added then add component and external position. 
+     * 1) If component is not already added then add component and external position.
      * 2) If component is added but no existing external position using the passed module exists then add the external position.
      * 3) If the existing position is being added to then just update the unit and data
      * 4) If the position is being closed and no other external positions or default positions are associated with the component
@@ -191,7 +191,7 @@ library PositionV2 {
      * @return                    Notional tracked balance
      */
     function getDefaultTrackedBalance(ISetToken _setToken, address _component) external view returns(uint256) {
-        int256 positionUnit = _setToken.getDefaultPositionRealUnit(_component); 
+        int256 positionUnit = _setToken.getDefaultPositionRealUnit(_component);
         return _setToken.totalSupply().preciseMul(positionUnit.toUint256());
     }
 

contracts/protocol/modules/v2/PerpV2BasisTradingModule.sol

@@ -47,6 +47,7 @@ import { PreciseUnitMath } from "../../../lib/PreciseUnitMath.sol";
 import { AddressArrayUtils } from "../../../lib/AddressArrayUtils.sol";
 import { UnitConversionUtils } from "../../../lib/UnitConversionUtils.sol";
 
+
 /**
  * @title PerpV2LeverageModuleV2
  * @author Set Protocol
@@ -60,8 +61,8 @@ import { UnitConversionUtils } from "../../../lib/UnitConversionUtils.sol";
  * Any pending funding costs or PnL is carried by the current token holders. To be used safely this module MUST issue using the
  * SlippageIssuanceModule or else issue and redeem transaction could be sandwich attacked.
  *
- * NOTE: The external position unit is only updated on an as-needed basis during issuance/redemption. It does not reflect the current
- * value of the Set's perpetual position. The current value can be calculated from getPositionNotionalInfo.
+ * NOTE: The external position unit is only updated on an as-needed basis during issuance, redemption, deposit and withdraw. It does not
+ * reflect the current value of the Set's perpetual position. The current value can be calculated from getPositionNotionalInfo.
  *
  * CHANGELOG:
  * - This contract has the same functionality as `PerpV2LeverageModule` but smaller bytecode size. It extends ModuleBaseV2 (which uses
@@ -352,6 +353,10 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
       onlyManagerAndValidSet(_setToken)
     {
         require(_collateralQuantityUnits > 0, "Deposit amount is 0");
+        require(
+            _collateralQuantityUnits <= _setToken.getDefaultPositionRealUnit(address(collateralToken)).toUint256(),
+            "Amount too high"
+        );
 
         uint256 notionalDepositedQuantity = _depositAndUpdatePositions(_setToken, _collateralQuantityUnits);
 
@@ -374,6 +379,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
       uint256 _collateralQuantityUnits
     )
       public
+      virtual
       nonReentrant
       onlyManagerAndValidSet(_setToken)
     {
@@ -385,7 +391,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
     }
 
     /**
-     * @dev MANAGER ONLY: Removes this module from the SetToken, via call by the SetToken. Deletes
+     * @dev SETTOKEN ONLY: Removes this module from the SetToken, via call by the SetToken. Deletes
      * position mappings associated with SetToken.
      *
      * NOTE: Function will revert if there is greater than a position unit amount of USDC of account value.
@@ -555,7 +561,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
     /**
      * @dev GOVERNANCE ONLY: Update max perpetual positions per SetToken. Only callable by governance.
      *
-     * @param _maxPerpPositionsPerSet       New max perpetual positons per set
+     * @param _maxPerpPositionsPerSet       New max perpetual positions per set
      */
     function updateMaxPerpPositionsPerSet(uint256 _maxPerpPositionsPerSet) external onlyOwner {
         maxPerpPositionsPerSet = _maxPerpPositionsPerSet;
@@ -830,12 +836,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
             initialCollateralPositionBalance
         );
 
-        _setToken.editExternalPosition(
-            address(collateralToken),
-            address(this),
-            _calculateExternalPositionUnit(_setToken),
-            ""
-        );
+        _updateExternalPositionUnit(_setToken);
 
         return collateralNotionalQuantity;
     }
@@ -885,12 +886,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
             initialCollateralPositionBalance
         );
 
-        _setToken.editExternalPosition(
-            address(collateralToken),
-            address(this),
-            _calculateExternalPositionUnit(_setToken),
-            ""
-        );
+        _updateExternalPositionUnit(_setToken);
 
         return collateralNotionalQuantity;
     }
@@ -1058,44 +1054,32 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
     }
 
     /**
-     * @dev Gets the mid-point price of a virtual asset from UniswapV3 markets maintained by Perp Protocol
+     * @dev Sets the external position unit based on PerpV2 Vault collateral balance. If there is >= 1
+     * position unit of USDC of collateral value, add the component and/or the external position to the
+     * SetToken. Else, untracks the component and/or removes external position from the SetToken. Refer
+     * to PositionV2#editExternalPosition for detailed flow.
      *
-     * @param  _baseToken           Address of virtual token to price
-     * @return price                Mid-point price of virtual token in UniswapV3 AMM market
-     */
-    function _calculateAMMSpotPrice(address _baseToken) internal view returns (uint256 price) {
-        address pool = perpMarketRegistry.getPool(_baseToken);
-        (uint160 sqrtPriceX96, , , , , , ) = IUniswapV3Pool(pool).slot0();
-        uint256 priceX96 = sqrtPriceX96.formatSqrtPriceX96ToPriceX96();
-        return priceX96.formatX96ToX10_18();
-    }
-
-    /**
-     * @dev Calculates the sum of collateralToken denominated market-prices of assets and debt for the Perp account per
-     * SetToken
+     * NOTE: Setting external position unit to the collateral balance is acceptable because, as noted above, the
+     * external position unit is only updated on an as-needed basis during issuance, redemption, deposit and
+     * withdraw. It does not reflect the current value of the Set's perpetual position. The true current value can
+     * be calculated from getPositionNotionalInfo.
      *
      * @param _setToken     Instance of SetToken
-     * @return int256       External position unit
      */
-    function _calculateExternalPositionUnit(ISetToken _setToken) internal view returns (int256) {
-        PerpV2Positions.PositionNotionalInfo[] memory positionInfo = getPositionNotionalInfo(_setToken);
-        uint256 positionLength = positionInfo.length;
-        int256 totalPositionValue = 0;
-
-        for (uint i = 0; i < positionLength; i++ ) {
-            int256 spotPrice = _calculateAMMSpotPrice(positionInfo[i].baseToken).toInt256();
-            totalPositionValue = totalPositionValue.add(
-                positionInfo[i].baseBalance.preciseMul(spotPrice)
-            );
-        }
+    function _updateExternalPositionUnit(ISetToken _setToken) internal {
+        int256 collateralValueUnit = perpVault.getBalance(address(_setToken))
+            .toPreciseUnitsFromDecimals(collateralDecimals)
+            .preciseDiv(_setToken.totalSupply().toInt256())
+            .fromPreciseUnitToDecimals(collateralDecimals);
 
-        int256 externalPositionUnitInPreciseUnits = _calculatePartialAccountValuePositionUnit(_setToken)
-            .add(totalPositionValue.preciseDiv(_setToken.totalSupply().toInt256()));
-
-        return externalPositionUnitInPreciseUnits.fromPreciseUnitToDecimals(collateralDecimals);
+        _setToken.editExternalPosition(
+            address(collateralToken),
+            address(this),
+            collateralValueUnit,
+            ""
+        );
     }
 
-
     /**
      * @dev Returns issuance or redemption adjustments in the format expected by `SlippageIssuanceModule`.
      * The last recorded externalPositionUnit (current) is subtracted from a dynamically generated
@@ -1107,7 +1091,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
      * @param _setToken                         Instance of the SetToken
      * @param _newExternalPositionUnit          Dynamically calculated externalPositionUnit
      * @return int256[]                         Components-length array with equity adjustment value at appropriate index
-     * @return int256[]                         Components-length array of zeroes (debt adjustements)
+     * @return int256[]                         Components-length array of zeroes (debt adjustments)
      */
     function _formatAdjustments(
         ISetToken _setToken,

contracts/protocol/modules/v2/PerpV2LeverageModuleV2.sol

@@ -271,7 +271,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
      * token market prices and needs to be generated on the fly to be meaningful.
      *
      * In the tables below, basePositionUnit = baseTokenBalance / setTotalSupply.
-     * 
+     *
      * As a user when levering, e.g increasing the magnitude of your position, you'd trade as below
      * | ----------------------------------------------------------------------------------------------- |
      * | Type  |  Action | Goal                      | `quoteBoundQuantity`        | `baseQuantityUnits` |
@@ -318,7 +318,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
         public
         nonReentrant
         onlyManagerAndValidSet(_setToken)
-    {        
+    {
         ActionInfo memory actionInfo = _createAndValidateActionInfo(
             _setToken,
             _baseToken,
@@ -413,7 +413,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
         // `positions[setToken]` mapping stores an array of addresses. The base token addresses are removed from the array when the
         // corresponding base token positions are zeroed out. Since no positions exist when removing the module, the stored array should
         // already be empty, and the mapping can be deleted directly.
-        delete positions[setToken]; 
+        delete positions[setToken];
 
         // Try if unregister exists on any of the modules
         address[] memory modules = setToken.getModules();
@@ -563,8 +563,8 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
 
     /**
      * @dev GOVERNANCE ONLY: Update max perpetual positions per SetToken. Only callable by governance.
-     * 
-     * @param _maxPerpPositionsPerSet       New max perpetual positons per set
+     *
+     * @param _maxPerpPositionsPerSet       New max perpetual positions per set
      */
     function updateMaxPerpPositionsPerSet(uint256 _maxPerpPositionsPerSet) external onlyOwner {
         maxPerpPositionsPerSet = _maxPerpPositionsPerSet;
@@ -1052,8 +1052,8 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
 
         int256 baseBalance = perpAccountBalance.getBase(address(_setToken), _baseToken);
         int256 basePositionUnit = baseBalance.preciseDiv(totalSupply.toInt256());
-        
-        int256 baseNotional = _baseQuantityUnits == basePositionUnit.neg() 
+
+        int256 baseNotional = _baseQuantityUnits == basePositionUnit.neg()
             ? baseBalance.neg()         // To close position completely
             : _baseQuantityUnits.preciseMul(totalSupply.toInt256());
 
@@ -1134,7 +1134,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
     function _syncPositionList(ISetToken _setToken) internal {
         address[] memory positionList = positions[_setToken];
         uint256 positionLength = positionList.length;
-        
+
         for (uint256 i = 0; i < positionLength; i++) {
             address currPosition = positionList[i];
             if (!_hasBaseBalance(_setToken, currPosition)) {
@@ -1236,7 +1236,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
      * @param _components                       Array of components held by the SetToken
      * @param _newExternalPositionUnit          Dynamically calculated externalPositionUnit
      * @return int256[]                         Components-length array with equity adjustment value at appropriate index
-     * @return int256[]                         Components-length array of zeroes (debt adjustements)
+     * @return int256[]                         Components-length array of zeroes (debt adjustments)
      */
     function _formatAdjustments(
         ISetToken _setToken,

deprecated/PerpV2LeverageModule.sol

@@ -254,7 +254,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
         setToken.address,
         {
           feeRecipient: owner.address,
-          performanceFeePercentage: ether(.0),
+          performanceFeePercentage: ether(.1),
           maxPerformanceFeePercentage: ether(.2)
         }
       );
@@ -294,6 +294,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
           baseToken,
           2,
           ether(.02),
+          true,
           true
         );
 
@@ -508,6 +509,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
           baseToken,
           2,
           ether(.02),
+          true,
           true
         );
 
@@ -542,7 +544,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
         // initialExternalPositionUnit = 10_000_000
         // finalExternalPositionUnit   =  9_597_857
 
-        const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);
+        const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);;
         expect(initialExternalPositionUnit).eq(usdcDefaultPositionUnit);
         expect(finalExternalPositionUnit).to.be.closeTo(expectedExternalPositionUnit, 1);
       });
@@ -855,6 +857,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
             baseToken,
             6,
             ether(.02),
+            true,
             true
           );
 
@@ -909,7 +912,12 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
 
       describe("when liquidation results in negative account value", () => {
         beforeEach(async () => {
+          // Move oracle price down, wait one day
+          await perpSetup.setBaseTokenOraclePrice(vETH, usdcUnits(10.5));
+          await increaseTimeAsync(ONE_DAY_IN_SECONDS);
+
           // Calculated leverage = ~8.5X = 8_654_438_822_995_683_587
+          // Lever again to track funding as `settled`
           await leverUp(
             setToken,
             perpBasisTradingModule,
@@ -918,9 +926,13 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
             baseToken,
             6,
             ether(.02),
+            true,
             true
           );
 
+          // Freeze funding changes
+          await perpSetup.clearingHouseConfig.setMaxFundingRate(ZERO);
+
           // Move oracle price down to 5 USDC to enable liquidation
           await perpSetup.setBaseTokenOraclePrice(vETH, usdcUnits(5.0));
 

test/integration/perpV2BasisTradingSlippageIssuance.spec.ts

@@ -991,7 +991,7 @@ describe("PerpV2LeverageSlippageIssuance", () => {
         // initialExternalPositionUnit = 10_000_000
         // finalExternalPositionUnit   =  9_597_857
 
-        const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);
+        const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);;
         expect(initialExternalPositionUnit).eq(usdcDefaultPositionUnit);
         expect(finalExternalPositionUnit).to.be.closeTo(expectedExternalPositionUnit, 1);
       });