QA Report

[code423n4]

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    AMM.sol.getPendingFundingPayment trader
    Oracle.sol.requireNonEmptyAddress _addr
    ClearingHouse.sol.whitelistAmm _amm

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    Oracle.sol, getUnderlyingPrice
    AMM.sol, updatePosition
    MarginAccount.sol, isLiquidatable
    Oracle.sol, getLatestRoundData
    AMM.sol, getCloseQuote

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

        Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

    MarginAccount.sol, 601, require(_liquidationIncentive <= PRECISION / 10, "MA.syncDeps.LI_GT_10_percent");

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    ClearingHouse.sol, updatePositions

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    InsuranceFund.withdraw
    VUSD.setMaxWithdrawalProcesses
    VUSD.withdraw
    VUSD.processWithdrawals

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    ClearingHouse._disperseLiquidationFee (liquidationFee)
    ClearingHouse.setParams (_tradeFee)
    ClearingHouse.initialize (_tradeFee)

Title: Duplicates in array Severity: Low Risk

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

ClearingHouse.whitelistAmm pushed (_amm) { emit MarketAdded(amms.length, _amm); amms.push(IAMM(_amm)); } VUSD.withdraw pushed (amount) { burn(amount); withdrawals.push(Withdrawal(msg.sender, amount)); }

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    Oracle.sol, initialize, 20
    ClearingHouse.sol, initialize, 35
    MarginAccount.sol, initialize, 121
    AMM.sol, initialize, 93

Title: Open TODOs Severity: Low Risk

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Open TODO in AMM.sol line 554 : // @todo handle case when totalPosition = 0

Open TODO in ClearingHouse.sol line 171 : // @todo put checks on slippage

Open TODO in MarginAccount.sol line 276 : @todo consider providing some incentive from insurance fund to execute a liquidation in this scenario.

Open TODO in AMM.sol line 141 : // sending market orders can fk the trader. @todo put some safe guards around price of liquidations

Title: Unbounded loop on array that can only grow can lead to DoS Severity: Low/Med Risk

A malicious attacker that is also a protocol owner can push unlimitedly to an array, that some function loop over this array. If increasing the array size enough, calling the function that does a loop over the array will always revert since there is a gas limit. This is a Med Risk issue since it can lead to DoS with a reasonable chance of having untrusted owner or even an owner that did a mistake in good faith.

    ClearingHouse.sol (L250): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L169): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L276): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L129): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L193): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L262): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled
    ClearingHouse.sol (L121): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm'] and can't be pulled

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    MarginAccount.sol (L432) buffer might be 0)
    AMM.sol (L251) _maker might be 0)
    AMM.sol (L551) takerPosition might be 0)
    MarginAccount.sol (L497) buffer might be 0)
    AMM.sol (L681) period might be 0)
    ClearingHouse.sol (L332) notionalPosition might be 0)
    MarginAccount.sol (L495) buffer might be 0)
    AMM.sol (L556) totalPosition might be 0)
    AMM.sol (L552) totalPosition might be 0)
    AMM.sol (L710) _underlyingPrice might be 0)
    AMM.sol (L251) maker might be 0)
    AMM.sol (L250) maker might be 0)
    AMM.sol (L703) _intervalInSeconds might be 0)
    AMM.sol (L250) _maker might be 0)
    AMM.sol (L592) baseAssetQuantity might be 0)

Title: Usage of an incorrect version of Ownbale library can potentially malfunction all onlyOwner functions Severity: High Risk

The current implementaion is using an non-upgradeable version of the Ownbale library.  instead of the upgradeable version: @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol.
A regular, non-upgradeable Ownbale library will make the deployer the default owner in the constructor. Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts. Therefore, there will be no owner when the contract is deployed as a proxy contract
Use @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol and @openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol instead.
And add     __Ownable_init(); at the beginning of the initializer.

    Oracle.sol
    AMM.sol

Title: Unbounded loop on array can lead to DoS Severity: High Risk

The attacker can push unlimitedly to an array, that some function loop over this array. If increasing the array size enough, calling the function that does a loop over the array will always revert since there is a gas limit. This is an High Risk issue since those arrays are publicly allows to push items into them.

    ClearingHouse.sol (L193): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L262): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L169): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L129): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L250): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L121): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']
    ClearingHouse.sol (L276): Unbounded loop on the array amms that can be publicly pushed by ['whitelistAmm']

[atvanguard]

Good findings.

[moose-code]

All findings grouped into this single report. Going to circle back to review the med/high submissions

[CloudEllie]

Noting here that judge has upgraded issue "Usage of an incorrect version of Ownbale library can potentially malfunction all onlyOwner functions" to a Medium risk finding, via issue #140