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

[0xSachinK]

External Audit Report

Fixes

Low Risk

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

    Informational

• [x] Design comments
  • [x] Return early if no funding will be withdrawn (2d9afda, 05694a0, e3cfd4d)
  • [x] Cache storage slot values (cd15170)
  • [x] 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

• [x] L393: Manager Only comment should be SetToken Only.

PerpV2BasisTradingModule

Imports

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

constructor

• [x] (nit): dev comment could contextualize logic.

initialize(setToken, feeState)

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

tradeAndTrackFunding

• [x] Naming consistency
  • BasisTradingModule tracks settled funding. So the function name is correct.
• [x] 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

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

moduleRedeemHook

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

updatePerformanceFee

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

getRedemptionAdjustments

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

_handleFees

• [x] (naming): _amount → _notionalFundingAmount for clarity

[0xSachinK]

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

[cgewecke]

:tada: This PR is included in version 0.9.2-hhat.1 :tada:

The release is available on:

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

Your semantic-release bot :package::rocket: