0x52 - When private keeper mode is off users can queue orders with the wrong asset

[sherlock-admin]

0x52

high

When private keeper mode is off users can queue orders with the wrong asset

Summary

After an order is initiated, it must be filled by calling resolveQueuedTrades. This function validates that the asset price has been signed but never validates that the asset being passed in matches the asset of the queuedTrade. When private keeper mode is off, which is the default state of the contract, this can be abused to cause huge loss of funds.

Vulnerability Detail

    for (uint32 index = 0; index < params.length; index++) {
        OpenTradeParams memory currentParams = params[index];
        QueuedTrade memory queuedTrade = queuedTrades[
            currentParams.queueId
        ];
        bool isSignerVerifed = _validateSigner(
            currentParams.timestamp,
            currentParams.asset,
            currentParams.price,
            currentParams.signature
        );
        // Silently fail if the signature doesn't match
        if (!isSignerVerifed) {
            emit FailResolve(
                currentParams.queueId,
                "Router: Signature didn't match"
            );
            continue;
        }
        if (
            !queuedTrade.isQueued ||
            currentParams.timestamp != queuedTrade.queuedTime
        ) {
            // Trade has already been opened or cancelled or the timestamp is wrong.
            // So ignore this trade.
            continue;
        }

        // If the opening time is much greater than the queue time then cancel the trad
        if (block.timestamp - queuedTrade.queuedTime <= MAX_WAIT_TIME) {
            _openQueuedTrade(currentParams.queueId, currentParams.price);
        } else {
            _cancelQueuedTrade(currentParams.queueId);
            emit CancelTrade(
                queuedTrade.user,
                currentParams.queueId,
                "Wait time too high"
            );
        }

        // Track the next queueIndex to be processed for user
        userNextQueueIndexToProcess[queuedTrade.user] =
            queuedTrade.userQueueIndex +
            1;
    }

BufferRouter#resolveQueueTrades never validates that the asset passed in for params is the same asset as the queuedTrade. It only validates that the price is the same, then passes the price and queueId to _openQueuedTrade:

function _openQueuedTrade(uint256 queueId, uint256 price) internal {
    QueuedTrade storage queuedTrade = queuedTrades[queueId];
    IBufferBinaryOptions optionsContract = IBufferBinaryOptions(
        queuedTrade.targetContract
    );

    bool isSlippageWithinRange = optionsContract.isStrikeValid(
        queuedTrade.slippage,
        price,
        queuedTrade.expectedStrike
    );

    if (!isSlippageWithinRange) {
        _cancelQueuedTrade(queueId);
        emit CancelTrade(
            queuedTrade.user,
            queueId,
            "Slippage limit exceeds"
        );

        return;
    }

    ...

    optionParams.totalFee = revisedFee;
    optionParams.strike = price;
    optionParams.amount = amount;

    uint256 optionId = optionsContract.createFromRouter(
        optionParams,
        isReferralValid
    );

Inside _openQueuedTrade it checks that the price is within the slippage bounds of the order, cancelling if its not. Otherwise it uses the price to open an option. According to documentation, the same router will be used across a large number of assets/pools, which means the publisher for every asset is the same, given that router only has one publisher variable.

Examples:

Imagine two assets are listed that have close prices, asset A = $0.95 and asset B = $1. An adversary could create an call that expires in 10 minutes on asset B with 5% slippage, then immediately queue it with the price of asset A. $0.95 is within the slippage bounds so it creates the option with a strike price of $0.95. Since the price of asset B is actually $1 the adversary will almost guaranteed make money, stealing funds from the LPs. This can be done back and forth between both pools until pools for both assets are drained.

In a similar scenario, if the price of the assets are very different, the adversary could use this to DOS another user by always calling queue with the wrong asset, causing the order to be cancelled.

Impact

Adversary can rug LPs and DOS other users

Code Snippet

https://github.com/sherlock-audit/2022-11-buffer/blob/main/contracts/contracts/core/BufferRouter.sol#L136-L185

Tool used

Manual Review

Recommendation

Pass the asset address through so the BufferBinaryOptions contract can validate it is being called with the correct asset

[IAm0x52]

Fixed in PR#2

Changes look good. The asset pair is now stored in BufferBinaryOptions and BufferRouter directly reads it from there instead of relying on user/keeper to supply the correct asset.