An admin can steal all users' USDC funds backing VUSD

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/VUSD.sol#L48-L67

Vulnerability details

The ProxyAdmin for all of the contracts is currently set to the governor address, which is a single-signature address. VUSD is a ERC20PresetMinterPauserUpgradeable which uses the shared ProxyAdmin.

Impact

After granting MINTER_ROLE to another address under its control, the governor can use this new address to mint() as many VUSD tokens as he/she wishes, withdraw() them, then call processWithdrawls() to withdraw all users' USDC.

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. Furthermore an exchange is not decentralized if it's possible to force a single person to shut things down by withdrawing all user funds. See these examples where similar findings have been flagged as high-severity issues.

Proof of Concept

    function withdraw(uint amount) external {
        burn(amount);
        withdrawals.push(Withdrawal(msg.sender, amount));
    }

    function processWithdrawals() external {
        uint reserve = reserveToken.balanceOf(address(this));
        require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');
        uint i = start;
        while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) {
            Withdrawal memory withdrawal = withdrawals[i];
            if (reserve < withdrawal.amount) {
                break;
            }
            reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
            reserve -= withdrawal.amount;
            i += 1;
        }
        start = i;
    }

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/VUSD.sol#L48-L67

    function __ERC20PresetMinterPauser_init_unchained(string memory, string memory) internal onlyInitializing {
        _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());

        _setupRole(MINTER_ROLE, _msgSender());
        _setupRole(PAUSER_ROLE, _msgSender());
    }

    /**
     * @dev Creates `amount` new tokens for `to`.
     *
     * See {ERC20-_mint}.
     *
     * Requirements:
     *
     * - the caller must have the `MINTER_ROLE`.
     */
    function mint(address to, uint256 amount) public virtual {
        require(hasRole(MINTER_ROLE, _msgSender()), "ERC20PresetMinterPauser: must have minter role to mint");
        _mint(to, amount);
    }

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/322d96706cd980b73ae29c8fed6f9469396ab09d/contracts/token/ERC20/presets/ERC20PresetMinterPauserUpgradeable.sol#L48-L67

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, disable the ability to grant the MINTER_ROLE, and therefore do not allow the margin contract address to change once it's set.

[atvanguard]

Duplicate of #40

[JeeberC4]

Grouping with warden's QA Report #32