Add a Timelock and an Event to ClearingHouse.sol:setParams()

[code423n4]

Lines of code

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

Vulnerability details

Impact

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

Here, no timelock capabilities nor events are implemented on ClearingHouse.sol:setParams(), which I believe is a function that can impact some users enough to make them want to react / be notified ahead of time, as it changes these 4 parameters: tradeFee, liquidationPenalty, maintenanceMargin and minAllowableMargin

Proof of Concept

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

File: ClearingHouse.sol
344:     function setParams(
345:         int _maintenanceMargin,
346:         int _minAllowableMargin,
347:         uint _tradeFee,
348:         uint _liquidationPenality
349:     ) external onlyGovernance {
350:         tradeFee = _tradeFee; 
351:         liquidationPenalty = _liquidationPenality;
352:         maintenanceMargin = _maintenanceMargin;
353:         minAllowableMargin = _minAllowableMargin;
354:     }

Recommended Mitigation Steps

Consider adding a timelock and an event to ClearingHouse.sol:setParams()
Additionally, I suggest adding an upperbound on tradeFee and liquidationPenalty, as too big of a possible value for those isn't good for trust.

[atvanguard]

Duplicate of #40

[moose-code]

Again gov can rug issue, but this is a very practical and useful suggestion 👍

[JeeberC4]

Grouping with warden's QA report #94