kgothatso - User can get front-run and loss funds and experience a DOS attack when they call `invoke`

[sherlock-admin3]

kgothatso

high

User can get front-run and loss funds and experience a DOS attack when they call invoke

Summary

Loss of funds because of size of array Invocation[]

Vulnerability Detail

1. User A calls invoke function with an array size of 100 and sends eth along with the call
2. User B sees this in the Mempool
3. User B calls invoke function with an array size of 1 and sends eth along with the call with more gas than user A
4. User B function call finishes before User A
5. User B gets sent all the eth in the contract including from user A because of this line of code https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L190
6. _invoke sends 0 eth for User A not what they sent to the contract
7. User A transaction will revert because the balance of eth in the contract is zero

Impact

Loss of funds ,DOS attack

Code Snippet

https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L140

https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L133

     
      function _invoke(address account, Invocation[] calldata invocations) private {
        if (msg.sender != account && !operators[account][msg.sender]) revert MultiInvokerUnauthorizedError();

        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 interfaceFee1,
                    InterfaceFee memory interfaceFee2
                ) = abi.decode(invocation.args, (IMarket, UFixed6, UFixed6, UFixed6, Fixed6, bool, InterfaceFee, InterfaceFee));

                _update(account, market, newMaker, newLong, newShort, collateral, wrap, interfaceFee1, interfaceFee2);
            } 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(account, vault, depositAssets, redeemShares, claimAssets, wrap);
            } else if (invocation.action == PerennialAction.PLACE_ORDER) {
                (IMarket market, TriggerOrder memory order) = abi.decode(invocation.args, (IMarket, TriggerOrder));

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

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

                _executeOrder(execAccount, 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.APPROVE) {
                (address target) = abi.decode(invocation.args, (address));

                _approve(target);
            }
        }
      
 @>       Address.sendValue(payable(account), address(this).balance);
    }

Tool used

Manual Review

Recommendation

Add a mapping state variable to store how much each user sends to the contract and send that value that was stored in the mapping for that user

[sherlock-admin3]

2 comment(s) were left on this issue during the judging contest.

z3s commented:

Invalid; Your assumption is wrong, there is no ETH from User A in User B's transaction.

FSchmoede commented:

Since concurrency (and race conditions) is not possible in EVM and solidity thus invalid.