[External Audit] Reentrancy in MultiInvoker due to calls to unauthenticated contracts

[arjun-io]

Description The MultiInvoker.sol contract makes external calls to the Market and Vault contracts. However, there’s no check that these are valid contracts registered with their respective factories.

Impact MultiInvoker.sol can be called with arbitrary contracts which can lead to unexpected reentrancy behavior.

Recommendations MultiInvoker.sol should check the provided market or vault address against MarketF actory.sol/VaultFactory.sol respectively to verify that it’s a valid instance.

[hrishibhat]

These issues are not included in the contest pool rewards

[arjun-io]

Fixed: https://github.com/equilibria-xyz/perennial-v2/pull/83

[MLON33]

From WatchPug,

https://github.com/equilibria-xyz/perennial-v2/blob/f216b2fd0ec45ea22b443a00fdfe2257f803ce7c/packages/perennial-extensions/contracts/MultiInvoker.sol#L312-L331

While we cannot see a way to exploit this Reentrancy to unauthenticated contracts, the fix did prevent calls to unauthentic vaults and markets.

However, the COMMIT_PRICE action can still be used to call an unauthenticated oracleProvider.

[arjun-io]

From WatchPug,

https://github.com/equilibria-xyz/perennial-v2/blob/f216b2fd0ec45ea22b443a00fdfe2257f803ce7c/packages/perennial-extensions/contracts/MultiInvoker.sol#L312-L331

While we cannot see a way to exploit this Reentrancy to unauthenticated contracts, the fix did prevent calls to unauthentic vaults and markets.

However, the COMMIT_PRICE action can still be used to call an unauthenticated oracleProvider.

Since the oracle factory has multiple levels of abstraction this change can be quite complex so while we recognize this is true, since there is no clear malicious use here we're going to not add the oracle check.