Due to incorrect msg.value handling, Users get DOSed everytime they execute KYBER_SWAP command more than once in a single batched call to execute() function.

[hats-bug-reporter[bot]]

Github username: @burhankhaja Twitter username: imaybeghost Submission hash (on-chain): 0x8974a7d5ab0b674835bc54c0c8ac5d24a54d7646adf7e9b782ca6eaa158f8593 Severity: high

Description: Bug Category\ Protocol insolvency

Description\ Users can't execute Commands.KYBER_SWAP more than once in a single batched call to execute() function, especially when the ether is required for both calls.

The root cause for this insolvency is that the entire msg.value sent to execute() is utilized in the first execution of Commands.KYBER_SWAP, as a result no ether is left to be sent for the another similar call in a batch.

Dispatcher:

 } else if (command == Commands.KYBER_SWAP) {
            (address tokenIn, uint256 amountIn, address tokenOut, , bytes memory targetData) = abi
                .decode(_inputs, (address, uint256, address, uint256, bytes));
            if (tokenOut == Constants.ETH) {
                revert AddressError();
            }
            if (tokenIn == Constants.ETH) {
                if (msg.value != amountIn) {
                    revert AmountError();
                }
            //@audit-issue here is the bug:
                (bool success, ) = kyberRouter.call{value: msg.value}(targetData); //@bug
                if (!success) {
                    revert CallFailed();
                }
            }

Vulnerable Scenario\ Imagine user spots some opportunity to profit from which require 2 batched payable calls to Commands.KYBER_SWAP

Lets say user has to send 2 ether on each call to Commands.KYBER_SWAP, which means user has to send 4 ethers as msg.value to router's execute() function.

Unfortunately all 4 ethers will be utilized in the first execution of Commands.KYBER_SWAP and there will be no ether left for another Commands.KYBER_SWAP command execution resulting in the protocol insolvency to handle different execution scenarios.

Suggestion\ Instead of passing entire msg.value, take a custom value from user input just like other parameters:

 } else if (command == Commands.KYBER_SWAP) {
-            (address tokenIn, uint256 amountIn, address tokenOut, , bytes memory targetData) = abi
-                 .decode(_inputs, (address, uint256, address, uint256, bytes));
+            (address tokenIn, uint256 amountIn, address tokenOut, , bytes memory targetData, uint msgValue) = abi
+                .decode(_inputs, (address, uint256, address, uint256, bytes, uint));
            if (tokenOut == Constants.ETH) {
                revert AddressError();
            }
            if (tokenIn == Constants.ETH) {
                if (msg.value != amountIn) {
                    revert AmountError();
                }
-                (bool success, ) = kyberRouter.call{value: msg.value}(targetData);
+                (bool success, ) = kyberRouter.call{value: msgValue}(targetData);
                if (!success) {
                    revert CallFailed();
                }
            }

Notice i created extra uint type variable msgValue that represents msg.value for the kyber_swap command

[yanisepfl]

Hello,

We classified this issue as Invalid because:

• It does not result in direct theft of user funds

That being said, indeed we cannot call twice the KYBER_SWAP command with ETH as tokenIn in the same execute call, but we do not consider that as an issue.

Thanks

[burhankhaja]

Szn Tu in his book "Blackhat protocols" advices that defi protocol must take security seriously and further states that all it takes is bunch of unsatisfied degens before the bankruptcy comes knocking at the door at 3:00 AM

Will definitely escalate this with love :heart: