Reentrancy vulnerability in the MultiInvoker smart contract due to calls to unauthenticated external contracts

Summary

reentrancy vulnerability in the MultiInvoker smart contract due to calls to unauthenticated external contracts. The vulnerability can potentially allow malicious contracts to reenter the MultiInvoker.

Vulnerability Detail

The vulnerability is related to the invoke function in the MultiInvoker contract. This function executes a series of actions provided in the invocations array, which can include calls to various external contracts. However, the invoke function does not validate or authenticate these external contracts before interacting with them, potentially allowing untrusted contracts to be called.

Impact

The impact of this vulnerability can be severe. Malicious or unauthenticated external contracts could exploit this reentrancy vulnerability to disrupt the normal operation of the MultiInvoker contract. This may lead to unintended state changes, financial losses, or other security risks within the contract.

Code Snippet

    function invoke(Invocation[] calldata invocations) external payable {
        for(uint i = 0; i < invocations.length; ++i) {
            Invocation memory invocation = invocations[i];

            if (invocation.action == PerennialAction.UPDATE_POSITION) {
                (
                    // update data
                    IMarket market,
                    UFixed6 newMaker,
                    UFixed6 newLong,
                    UFixed6 newShort,
                    Fixed6 collateral,
                    bool wrap,
                    InterfaceFee memory interfaceFee
                ) = abi.decode(invocation.args, (IMarket, UFixed6, UFixed6, UFixed6, Fixed6, bool, InterfaceFee));

                _update(msg.sender, market, newMaker, newLong, newShort, collateral, wrap, interfaceFee);
            } else if (invocation.action == PerennialAction.UPDATE_VAULT) {
                (IVault vault, UFixed6 depositAssets, UFixed6 redeemShares, UFixed6 claimAssets, bool wrap)
                    = abi.decode(invocation.args, (IVault, UFixed6, UFixed6, UFixed6, bool));

                _vaultUpdate(vault, depositAssets, redeemShares, claimAssets, wrap);
            } else if (invocation.action == PerennialAction.PLACE_ORDER) {
                (IMarket market, TriggerOrder memory order) = abi.decode(invocation.args, (IMarket, TriggerOrder));

                _placeOrder(msg.sender, market, order);
            } else if (invocation.action == PerennialAction.CANCEL_ORDER) {
                (IMarket market, uint256 nonce) = abi.decode(invocation.args, (IMarket, uint256));

                _cancelOrder(msg.sender, market, nonce);
            } else if (invocation.action == PerennialAction.EXEC_ORDER) {
                (address account, IMarket market, uint256 nonce)
                    = abi.decode(invocation.args, (address, IMarket, uint256));

                _executeOrder(account, market, nonce);
            } else if (invocation.action == PerennialAction.COMMIT_PRICE) {
                (address oracleProviderFactory, uint256 value, bytes32[] memory ids, uint256 version, bytes memory data, bool revertOnFailure) =
                    abi.decode(invocation.args, (address, uint256, bytes32[], uint256, bytes, bool));

                _commitPrice(oracleProviderFactory, value, ids, version, data, revertOnFailure);
            } else if (invocation.action == PerennialAction.LIQUIDATE) {
                (IMarket market, address account, bool revertOnFailure) = abi.decode(invocation.args, (IMarket, address, bool));

                _liquidate(market, account, revertOnFailure);
            } else if (invocation.action == PerennialAction.APPROVE) {
                (address target) = abi.decode(invocation.args, (address));

                _approve(target);
            }

            // Eth must not remain in this contract at rest
            payable(msg.sender).transfer(address(this).balance);
        }
    }

https://github.com/sherlock-audit/2023-10-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L115C4-L169C1

Tool used

Manual Review

Recommendation

Implement proper authentication and validation checks for external contracts called within the invoke function. Ensure that only trusted and authenticated contracts are allowed to interact with the MultiInvoker. Additionally, consider implementing a strict access control mechanism to prevent unauthorized access to the invoke function and closely review the logic of each external contract call to ensure that reentrancy vulnerabilities are addressed. Regular security audits and testing are also recommended to identify and rectify potential issues.

sherlock-audit / 2023-10-perennial-judging

0xVinylDavyl - Reentrancy vulnerability in the MultiInvoker smart contract due to calls to unauthenticated external contracts #3