ClearingHouse fail if broken amm whitelisted

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/ClearingHouse.sol#L339

Vulnerability details

Impact

Governance can whitelist amm in ClearingHouse using the whitelistAmm function. Since a lot of function in the ClearingHouse contract will iterate each of the amm in amms and call various amm.function(), if a broken amm is whitelisted or became broken, many of those functions will revert. There are no mechanism to remove amm from the amms array so the protocol will fail.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/ClearingHouse.sol#L339

    function whitelistAmm(address _amm) external onlyGovernance {
        emit MarketAdded(amms.length, _amm);
        amms.push(IAMM(_amm));
    }

Recommended Mitigation Steps

1. Add a way to remove amm from the amms array
2. Check basic functionality of amm before adding

[atvanguard]

Acknowledging but yes system heavily relies on the admins to do the right thing, the right way. Would classify this as 0 (Informational).

[JasoonS]

Listed mitigation steps are valid. I'd also recommend implementing them (2 can be tricky, something like https://eips.ethereum.org/EIPS/eip-165 isn't exactly a joy...).

Basically mistakes can happen and so best have safeguards (especially if they are easy to implement).

[JasoonS]

Changing to Low risk in light of #66 - which is the bigger risk (although same/similar mitigation steps)

[JeeberC4]

Grouping with warden's QA report #89