Unprotected slippage tolerance can lead to user/protocol loss of funds

Summary

If a bad actor specifies a malicious oracle, the values returned by $oracle.getQuoteAmounts(quoteParams_) may be higher than expected, and as a result, the protocol may end up transferring more funds from the user to the beneficiary.

Vulnerability Detail

The createSwapper() function is used to create a new instance of the Swapper contract.Within the function, the params_.oracleParams is parsed into an OracleImpl instance and assigned to a local variable called oracle.

function _parseIntoOracle(OracleParams calldata oracleParams_) returns (OracleImpl) {
    if (address(oracleParams_.oracle)._isNotEmpty()) {
        return oracleParams_.oracle;
    } else {
        // if oracle not provided, create one with provided params
        CreateOracleParams calldata createOracleParams = oracleParams_.createOracleParams;
        return createOracleParams.factory.createOracle(createOracleParams.data);
    }

}

As the code above, this function checks if an Oracle instance is provided in the input parameters. If it is not provided, then it creates a new Oracle instance with the provided initialization data using the factory contract. The function returns the OracleImpl instance in either case.

After the Swapper contract is created, third parties can calls the flash() to withdraw tokens in return for sending tokenToBeneficiary to beneficiary.

Within the function, the amountsToBeneficiary array variable is assigned the result of calling the $oracle.getQuoteAmounts function with the quoteParams_ input parameter.The function also checks whether the balance of tokenToTrader in the contract is greater than or equal to amountToTrader. If it is not, the function throws an InsufficientFunds_InContract exception. If the balance is sufficient, the function adds amountsToBeneficiary[i] to amountToBeneficiary and transfers amountToTrader of tokenToTrader to msg.sender using the _safeTransfer function.

function _transferToTrader(address tokenToBeneficiary_, OracleImpl.QuoteParams[] calldata quoteParams_)
        internal
        returns (uint256 amountToBeneficiary, uint256[] memory amountsToBeneficiary)
    {
        amountsToBeneficiary = $oracle.getQuoteAmounts(quoteParams_);
        uint256 length = quoteParams_.length;
        if (amountsToBeneficiary.length != length) revert Invalid_AmountsToBeneficiary();

        uint128 amountToTrader;
        address tokenToTrader;
        for (uint256 i; i < length;) {
            OracleImpl.QuoteParams calldata qp = quoteParams_[i];

            if (tokenToBeneficiary_ != qp.quotePair.quote) revert Invalid_QuoteToken();
            tokenToTrader = qp.quotePair.base;
            amountToTrader = qp.baseAmount;

            if (amountToTrader > tokenToTrader._balanceOf(address(this))) {
                revert InsufficientFunds_InContract();
            }

            amountToBeneficiary += amountsToBeneficiary[i];
            tokenToTrader._safeTransfer(msg.sender, amountToTrader);

            unchecked {
                ++i;
            }
        }
    }

However, if a bad actor specifies a malicious oracle, the values returned by $oracle.getQuoteAmounts(quoteParams_) may be higher than expected, and as a result, the protocol may end up transferring more funds from the user to the beneficiary.

Impact

Due to the bad oracle's quote, the user may lose a significant amount of funds.

Code Snippet

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

Tool used

Manual Review

Recommendation

Require maximum values that user is willing to accept for a slippage tolerance

Duplicate of #37

sherlock-audit / 2023-04-splits-judging

Bauer - Unprotected slippage tolerance can lead to user/protocol loss of funds #32