Reentrancy in MultiInvoker due to calls to unauthenticated contracts
Description
The MultiInvoker is a contract that allows end users to atomically compose several Market and Vault calls into a single transaction, saving gas and ensuring safety because the user can be assured that no other transactions can run between their sequence of transactions.
In order to do this, it makes external calls to other contracts, including Market, Vault, and DSU. However, there is no check in MultiInvoker that the addresses supplied are valid contracts registered with their respective factories.
Impact
MultiInvoker can be called with arbitrary contracts, which can lead to unexpected reentrancy behavior.
Recommendations
MultiInvoker should check the provided market or vault address against MarketFactory/VaultFactory respectively to verify that it is a valid instance.
Remediation
This finding was acknowledged and a fix was implemented in commit with the addition of the following two modifiers:
/// @notice Target market must be created by MarketFactory
modifier isMarketInstance(IMarket market) {
if(!marketFactory.instances(market))
revert MultiInvokerInvalidInstanceError();
_;
}
/// @notice Target vault must be created by VaultFactory
modifier isVaultInstance(IVault vault) {
if(!vaultFactory.instances(vault))
revert MultiInvokerInvalidInstanceError();
_;
}