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

[0xSachinK]

External Audit Report

Fixes

Low Risk

• Base functions are not overridden (Decreased bytecode size in 0d70785, Fix in c857a3f)
• Subtraction underflow when calling deposit
  • Todo: Gain more clarity
  • Proposed solution below, added in 910fb59
• Fix update external position unit
  • _updateWithdrawFundingState should also update the external position unit

Informational

• Design comments
  • Return early if no funding will be withdrawn (2d9afda, 05694a0, e3cfd4d)
  • Cache storage slot values (cd15170)
  • Spelling and grammar (1e863be, fa3898e, c71d823)

Explained: Subtraction underflow when calling deposit

I was not able to recreate the bug even after depositing the entire withdrawn amount for >500 times consecutively. But as bernard pointed out in our chat, he was able to recreate the issue without any withdrawing and just depositing airdropped tokens.

In this script that bernard shared, he

• creates a SetToken with 100 USDC as a position unit.
• Issues 10 such Sets.
• Then airdrops 100 USDC to the Set.
• Then calls deposit with (balance/totalSupply = 1100/10 = 110) units.
  • In the deposit function, we calculate depositNotional = 110 * totalsupply = 100 * 10 = 1100, and then invoke deposit on PerpV2, which pulls 1100 USDC from the Set.
  • But, here the deposit function should have reverted because we are trying to deposit more than what we are tracking as position units (that is we are also depositing the airdropped amount).
  • But since we don't catch that here, we end up with a subtraction overflow when we are trying to edit the default position unit. The calculateDefaultEditPositionUnit is designed to not pick up airdropped amounts in its calculations.
  • In this particular example, calculateDefaultEditPositionUnit is passed the following parameters,
    • _setTokenSupply: 10e18
    • _preTotalNotional: 1100e6
    • _postTotalNotional: 0
    • _prePositionUnit: 100e6
    • And error occurs because, airdroppedAmount (1100 - 100 * 10)e6 = 100e6 is greater than _postTotalNotional (0).

Proposed Solution

Rather than modifying the PositionV2#calculateDefaultEditPositionUnit, we fix the PerpV2LeverageModuleV2#deposit unit to check that the deposit position units is <= current position unit.

Internal Audit Report

All changes added in 90b0276 and 56bfe82.

PerpV2LeverageModuleV2

• L393: Manager Only comment should be SetToken Only.

PerpV2BasisTradingModule

Imports

• (nit): Separate IERC20 (imported from Zeppelin) from other imports and place at top of file

constructor

• (nit): dev comment could contextualize logic.

initialize(setToken, feeState)

• (nit/docs): _settings param is not documented

tradeAndTrackFunding

• Naming consistency
  • BasisTradingModule tracks settled funding. So the function name is correct.
• onlyManagerAndValidSet checks are duplicated by call to PerpV2LeverageModule.trade. (For some methods this contract falls back on base modifiers, in others it doesn’t)
  • Whenever we call an internal function that uses a passed in parameter (eg. _updateSetlledFunding(_setToken) utilizes _setToken) we first ensure the parameter is valid (e.g. validate _setToken via onlyManagerAndValidSet modifier). But if we are calling the base function (PerpV2LeverageModule.###) at the very top in the function, example in removeModule then we don't duplicate the modifiers.

withdrawFundingAndAccrueFees

• javadocs: Added note
• method _withdraw is called without using PerpV2LeverageModuleV2 predicate.
  • All internal functions are called without using PerpV2LeverageModuleV2 predicate

moduleRedeemHook

• Could newExternalPositionUnit on L319 ever end up being negative?
  • Added check and set to zero when negative

updatePerformanceFee

• describe procedure for settling funding to zero before updating fee
• inconsistent require condition

getRedemptionAdjustments

• can this position unit ever go negative (Fixed)
• Could logic be refactored so that this method and moduleRedeemHook share the core settledFunding → trade → newPositionUnit flow?

_handleFees

• (naming): _amount → _notionalFundingAmount for clarity

[cgewecke]

We only call this as part of the external deposit and withdraw flows. If we change it to be an approximation of the position unit (b/c the accountValue is a function of the oracle price, rather than AMM state) and recommend that people calculate "the current value ... from getPositionNotionalInfo" (e.g off-chain?) maybe we should just stop editing the positionUnit on deposit and withdraw altogether. What use case does it serve?

Would this have any implications for the strategy extensions going forward? (It looks like the LeverageStrategy contract doesn't read the position unit anywhere.)

[0xSachinK]

If you take a look at the PositionV2#editExternalPosition function, it does the following

1. It adds/removes the component to SetToken's components list.
2. It adds/removes the associated module to SetToken's external modules list.
3. It updates the external position unit.

1, and 2 are required for the rest of the stack. Something like below would also acheive 1 and 2.

  _setToken.editExternalPositionUnit(
      address(collateralToken),
      address(this),
      1 if account value unit in PerpV2 > 0 else 0  // pseudocode
  );

The fact is that, there is no THE correct value that we can set here. Each value is merely a placeholder, which helps to achieve 1 and 2. The actual value is determined only during issuance/redemption. But it's fairly simple to fetch the account value from Perp and is very close to the values we were calculating earlier. Hence fetching the account value seems to be a sweet spot to me.

[cgewecke]

Yes. I wonder if your suggestion to toggle based on the vault balance might be the right thing to do - it seems closer to what's intended than either of the account valuation strategies.

[0xSachinK]

Update: Instead of toggling we are setting the unit to collateralValueUnit. As we want the module to remain added to the SetToken as long as there is collateral in PerpV2 vault for the Set's account. In contrast, account value doesn't provide the same guarantee because it can flip negative (even when collateral balance > 0) after liquidations.

    function _updateExternalPositionUnit(ISetToken _setToken) internal {
        int256 collateralValueUnit = perpVault.getBalance(address(_setToken))
            .toPreciseUnitsFromDecimals(collateralDecimals)
            .preciseDiv(_setToken.totalSupply().toInt256())
            .fromPreciseUnitToDecimals(collateralDecimals);

        _setToken.editExternalPosition(
            address(collateralToken),
            address(this),
            collateralValueUnit,
            ""
        );
    }

[0xSachinK]

Rebase to master to pull in the latest integration tests merged to master.

[cgewecke]

I think this all looks good! Solution to airdrop problem really nice, makes sense.

Should _updateWithdrawFundingState also update the external position unit?

Idk if that question is still open but seems fine to me.

Have added some small fixes for the things we discussed April 12 in #243. If those look ok to you then merging them into this branch should resolve the remaining CI blockers here and this is done.

(Am tentatively approving on the assumption you'll pull those in or do something equivalent)

[cgewecke]

🎉 This PR is included in version 0.9.2-hhat.1 🎉

The release is available on:

• npm package (@hardhat dist-tag)
• GitHub release

Your semantic-release bot 📦🚀