An admin can rug pull by changing contract addresses

[code423n4]

Lines of code

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

Vulnerability details

Impact

By implementing malicious versions of the interfaces required by an IRegistry, the single-sig governor admin address can rug pull funds.

Even if the governor is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. In addition the single private key may be taken in a hack. See this example where a similar finding has been flagged as a high-severity issue.

Proof of Concept

By silently changing the oracle of each AMM...

    function syncDeps(address _registry) public onlyGovernance {
        IRegistry registry = IRegistry(_registry);
        clearingHouse = registry.clearingHouse();
        oracle = IOracle(registry.oracle());
    }

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L730-L734

...the oracle.getUnderlyingTwapPrice() can be made to return a value favorable to the attacker in all calls to settleFunding(), draining funds

    function getUnderlyingTwapPrice(uint256 _intervalInSeconds) public view returns (int256) {
        return oracle.getUnderlyingTwapPrice(underlyingAsset, _intervalInSeconds);
    }

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L387-L389

By silently changing the marginAccount of the InsuranceFund...

    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

...the marginAccount can be made to receive all pending obligation funds

    function settlePendingObligation() public {
        if (pendingObligation > 0) {
            uint toTransfer = Math.min(vusd.balanceOf(address(this)), pendingObligation);
            if (toTransfer > 0) {
                pendingObligation -= toTransfer;
                vusd.safeTransfer(marginAccount, toTransfer);
            }
        }
    }

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

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 a multisig either.

Tools Used

Code inspection
Hardhat
snowtrace.io

Recommended Mitigation Steps

Use a multisig and disable the ability to change contract addresses

[atvanguard]

Acknowledging. System heavily relies on the admins to do the right thing, the right way. It's a design choice. Moreover, the governance contract (already voted and created by the DAO) is controlled by reputed members from the ecosystem.

Would classify this as 0 (Informational).

[moose-code]

@JasoonS lets go through and settle on this one. Further thinking of downgrading to low/informational. Simply the presence of upgradeable smart contracts means everything here plus more is obviously possible by admin.

At the early stage of system development, this is a design choice often taken while many changes still need to likely occur to the system. It is true that timelocks and admin multisigs and other precautions can be put in place. Sounds like the admin is a multisig anyways.

[moose-code]

Judging all "admin keys can rug or set funny values" as low severity. This is less helpful for the sponsor

[JeeberC4]

Grouping with warden's QA Report #32