obront - Owner can steal accumulated `payback`

[sherlock-admin]

obront

medium

Owner can steal accumulated payback

Summary

If any ETH is sent to the payback() function with the intention to accumulate until the next flash() call (as described in the code comments), it can be stolen by the owner of the Swapper in advance of the swap being performed.

Vulnerability Detail

The SwapperImpl.sol contract implements a payback() function that is used by swappers if $tokenToBeneficiary == ETH to pay back the amountToBeneficiary after performing the swap.

/// allows flash to track eth payback to beneficiary
/// @dev if used outside swapperFlashCallback, msg.sender may lose funds
/// accumulates until next flash call
function payback() external payable {
    $_payback += msg.value.toUint96();
    emit Payback(msg.sender, msg.value);
}

According to the comments, the payback() function is intended to accumulate until the next flash call. This feature can be used by a swapper to pay back some of the ETH in advance, and only later perform the flash call to receive the assets.

However, if a swapper were ever to use this feature, their ETH could be stolen by the owner of the Swapper.

That's because SwapperImpl inherits from WalletImpl, which has the following function:

function execCalls(Call[] calldata calls_)
    external
    payable
    onlyOwner
    returns (uint256 blockNumber, bytes[] memory returnData)
{
    blockNumber = block.number;
    uint256 length = calls_.length;
    returnData = new bytes[](length);

    bool success;
    for (uint256 i; i < length;) {
        Call calldata calli = calls_[i];
        // @ok any gas stuff here?
        // - don't think so since owner is calling, and if it fails we get !success
        // @ok eoas will succeed here - so what?
        (success, returnData[i]) = calli.to.call{value: calli.value}(calli.data);
        require(success, string(returnData[i]));

        unchecked {
            ++i;
        }
    }

    emit ExecCalls(calls_);
}

This function allows the owner of the Swapper to perform any arbitrary action on the Swapper's behalf, including sending its ETH.

If the owner removes all ETH from the contract, then when the swapper does finally perform its action to take the assets, the following subtraction will underflow and fail:

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

As a result, the swapper will not be able to receive their assets, and the owner will have stolen their ETH.

Impact

Any swappers who payback() ETH in advance, expecting it to accumulate in the contract as specified in the comments, can have their ETH stolen.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

The SwapperImpl.sol contract should not inherit from WalletImpl — it gives the owner too much power.

Alternatively, the comment should be removed that tells swappers that they can send ETH that will accumulate until the next flash call, as they should know that any ETH they deposit to the contract outside of the context of a flash() call can be stolen.

[ctf-sec]

Protocol already use a comment to acknowledge the issue + swapper owner is considered trusted:

Q: Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED? no protocol owner(s); oracle, swapper, & pass-through-wallet owners are TRUSTED