search

sherlock-audit / 2023-04-hubble-exchange-judging

7 stars 6 forks source link

0x52 - MarginAccountHelper will be bricked if registry.marginAccount or insuranceFund ever change #170

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

0x52

medium

MarginAccountHelper will be bricked if registry.marginAccount or insuranceFund ever change

Summary

MarginAccountHelper#syncDeps causes the contract to refresh it's references to both marginAccount and insuranceFund. The issue is that approvals are never made to the new contracts rendering them useless.

Vulnerability Detail

MarginAccountHelper.sol#L82-L87

function syncDeps(address _registry) public onlyGovernance {
    IRegistry registry = IRegistry(_registry);
    vusd = IVUSD(registry.vusd());
    marginAccount = IMarginAccount(registry.marginAccount());
    insuranceFund = IInsuranceFund(registry.insuranceFund());
}

When syncDeps is called the marginAccount and insuranceFund references are updated. All transactions require approvals to one of those two contract. Since no new approvals are made, the contract will become bricked and all transactions will revert.

Impact

Contract will become bricked and all contracts that are integrated or depend on it will also be bricked

Code Snippet

MarginAccountHelper.sol#L82-L87

Tool used

Manual Review

Recommendation

Remove approvals to old contracts before changing and approve new contracts after

0xshinobii commented 1 year ago

Valid issue but we will be using syncDeps mainly during the deployment. Later on, since both marginAccount and insuranceFund are upgradeable contracts, their address won't change.