[WP-H7] InsuranceFund#syncDeps() may cause users' fund loss

[code423n4]

Lines of code

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

Vulnerability details

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

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

The Governance address can call InsuranceFund.sol#syncDeps() to change the contract address of vusd anytime.

However, since the tx to set a new address for vusd can get in between users' txs to deposit and withdraw, in some edge cases, it can result in users' loss of funds.

PoC

1. Alice deposited 1,000,000 VUSD to InsuranceFund;
2. Gov called syncDeps() and set vusd to the address of VUSDv2;
3. Alice called withdraw() with all the shares and get back 0 VUSDv2.

As a result, Alice suffered a fund loss of 1,000,000 VUSD.

Recommendation

1. Consider making vusd unchangeable;
2. If a possible migration of vusd must be considered, consider changing the syncDeps() to:

function syncDeps(IRegistry _registry) public onlyGovernance {
    uint _balance = balance();
    vusd = IERC20(_registry.vusd());
    require(balance() >= _balance);
    marginAccount = _registry.marginAccount();
}

[atvanguard]

Acknowledging but yes system heavily relies on the admins to do the right thing, the right way. We might remove several such upgradeability rights during a broader refactor of the entire system.

[moose-code]

Downgrading to medium as this is largely admin related.