QA Report

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/ClearingHouse.sol#L344-L354
https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/legos/Governable.sol#L15-L21

Vulnerability details

When fees/margin or governance are involved it's important to emit events for off-chain monitors/tools to be able to react if necessary.

Impact

Automated tools especially need all relevant ancillary data to be emitted in order to efficiently react to it. An automated bot trading with the hubble exchange will not be able to see changes to fee/margin changes in real time, and may submit orders which cause it to miscalculate P&L, causing it to lose capital. This is especially true because the contracts do not use timelocks for changes. See these examples where similar findings have been flagged as medium/high-severity issues.

Proof of Concept

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

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

    function setGovernace(address _governance) external onlyGovernance {
        _setGovernace(_governance);
    }

    function _setGovernace(address _governance) internal {
        governance = _governance;
    }

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/legos/Governable.sol#L15-L21

The onlyGovernance modifier does not emit events

    address public governance;

    modifier onlyGovernance() {
        require(msg.sender == governance, "ONLY_GOVERNANCE");
        _;
    }

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/legos/Governable.sol#L8-L13

The provided deployment script only uses a signer rather than a contract as the governance address. Furthermore, the live environment deployed on testnet has a deployed InsuranceFund which uses the onlyGovernance modifier...

    function syncDeps(IRegistry _registry) public onlyGovernance {
        vusd = IERC20(_registry.vusd());
        marginAccount = _registry.marginAccount();
    }

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/InsuranceFund.sol#L116-L119

...and the only transaction interacting with this function appears here and is called by an address, not a contract. There are no other transactions to the insurance fund to change the governance address, so it's clear that the testnet does not use an emitting governor either.

Tools Used

Code inspection
Hardhat
snowtrace.io

Recommended Mitigation Steps

Emit events for these changes

[atvanguard]

Severity is 0.

[moose-code]

Dup of #31

[JeeberC4]

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.
The original title, for the record, was Missing events for critical governance and fee/margin rate changes