tradeFee and liquidationPenalty can be set to any value > 0

[code423n4]

Lines of code

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

Vulnerability details

Impact

If tradeFee or liquidationPenalty is set above PRECISION(1e6), it will charge more than entire trade amount as fee which will lead to negative margin of the user.
If tradeFee+liquidationPenalty is set above maintenanceMargin, the user will have negative margin after liquidation.
If tradeFee is set above maintenanceMargin, it is possible for the user to have negative margin after closing position.

Proof of Concept

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

    function setParams(
        int _maintenanceMargin,
        int _minAllowableMargin,
        uint _tradeFee,
        uint _liquidationPenality
    ) external onlyGovernance {
        tradeFee = _tradeFee;
        liquidationPenalty = _liquidationPenality;
        maintenanceMargin = _maintenanceMargin;
        minAllowableMargin = _minAllowableMargin;
    }

Recommended Mitigation Steps

To make sure the protocol remains functional, hard rules should be set to avoid the above situation in setParams. To reduce centralization risk, it is also advised a max fee being hardcoded to the contract (e.g. 10%).

[atvanguard]

Duplicate of #40

[moose-code]

Would say slight difference to 40. This is more useful and not just governance can rug users. Given these magic 1e6 number used in calcs, it is very advisable to have hard upper limits set to avoid a botched call by admin

[JeeberC4]

Grouping with warden's QA report #89