flash() does not account for $_payback properly, as a result, transferToBeneficiary() might revert even though there is sufficient balance to cover ``amountToBeneficiary``.

Summary

flash() does not account for $_payback properly, as a result, transferToBeneficiary() might revert even though theis is sufficient balance to cover ``amountToBeneficiary``.

Vulnerability Detail

flash() allows third parties to withdraw tokens in return for sending tokenToBeneficiary to beneficiary.

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-swapper/src/SwapperImpl.sol#L203-L221

Since the function is payable, it allows the user to send eth to the contract as payback, however, in contrast to the function payback(), which keeps track of the amount in $_payback, the function does not change the value of $_payback.

  function payback() external payable {
        $_payback += msg.value.toUint96();
        emit Payback(msg.sender, msg.value);
    }

Meanwhile, the function calls _transferToBeneficiary() to send tokens to the beneficiary.

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-swapper/src/SwapperImpl.sol#L218

Since $_payback is not properly accounted above, when the tokens to be sent is ETH, the condition $_payback < amountToBeneficiary_ is not properly tested. As a result, even though there is enough balance to cover amountToBeneficiary_, the function might still revert.

 if (tokenToBeneficiary_._isETH()) {
            if ($_payback < amountToBeneficiary_) {
                revert InsufficientFunds_FromTrader();
            }
            $_payback = 0;

            // send eth to beneficiary
            uint256 ethBalance = address(this).balance;
            excessToBeneficiary = ethBalance - amountToBeneficiary_;
            _beneficiary.safeTransferETH(ethBalance);

Impact

$_payback is not properly accounted in flash(), as a result, _transferToBeneficiary() might fail even though there is sufficient balance to cover amountToBeneficiary_.

Code Snippet

See above

Tool used

VScode

Manual Review

Recommendation

Keep track of $_payback properly in flash():

function flash(OracleImpl.QuoteParams[] calldata quoteParams_, bytes calldata callbackData_)
        external
        payable
        pausable
    {

+       $_payback += msg.value.toUint96();
+        emit Payback(msg.sender, msg.value);

        address _tokenToBeneficiary = $tokenToBeneficiary;
        (uint256 amountToBeneficiary, uint256[] memory amountsToBeneficiary) =
            _transferToTrader(_tokenToBeneficiary, quoteParams_);

        ISwapperFlashCallback(msg.sender).swapperFlashCallback({
            tokenToBeneficiary: _tokenToBeneficiary,
            amountToBeneficiary: amountToBeneficiary,
            data: callbackData_
        });

        uint256 excessToBeneficiary = _transferToBeneficiary(_tokenToBeneficiary, amountToBeneficiary);

        emit Flash(msg.sender, quoteParams_, _tokenToBeneficiary, amountsToBeneficiary, excessToBeneficiary);
    }

Duplicate of #50

sherlock-audit / 2023-04-splits-judging

chaduke - flash() does not account for $_payback properly, as a result, _transferToBeneficiary() might revert even though there is sufficient balance to cover ``amountToBeneficiary_``. #87