high

Loss of funds in SwapperImpl

Summary

Loss of user's ETH in SwapperImpl.flash()

Vulnerability Detail

Function SwapperImpl.flash() calls function SwapperImpl._transferToBeneficiary() And there is check for $_payback variable if token is ETH:

            if ($_payback < amountToBeneficiary_) {
                revert InsufficientFunds_FromTrader();
            }
            // @audit high если amount меньше, чем _payback, то мы его занулим, но при этом не весь эфир отправим
            $_payback = 0;

In the code above you just check that your $_payback value is more or equal to amountToBeneficiary_ And then you set it to 0. But the correct behaviour should be:

            $_payback = $_payback - amountToBeneficiary_;

Because you withdraw only amountToBeneficiary_, not total $_payback value of ETH

Impact

Loss of funds. E.g.:

Alice creates SwapperImpl with $tokenToBeneficiary = ETH and accumulates her business revenue there
She has 1 ETH on her balance
Let's imagine that quote token is USDC and current ETH price is 1000 USDC
Malicious user calls SwapperImpl.flash() and wants to change his 500 USDT to 0.5 ETH from contract balance, so amountToBeneficiary_ = 0.5e18 and $_payback = 1e18
$_payback is more than amountToBeneficiary_ so all checks passed
But then you set $_payback to 0
Alice gets her 0.5 ETH because she is $beneficiary
Now she wants to withdraw the rest of her ETH (0.5 ETH) and she can't do it, because now the check $_payback >= amountToBeneficiary_ fails

Result: Alice lost her funds

Code Snippet

https://github.com/sherlock-audit/2023-04-splits/blob/7303cc26205f10ca9111be31f3574d2573df92b1/splits-swapper/src/SwapperImpl.sol#L266

Tool used

Manual Review

Recommendation

Remove $_payback variable Use just current address(this).balance. It's enough

Duplicate of #50

sherlock-audit / 2023-04-splits-judging

R2 - Loss of funds in SwapperImpl #114