Description

Multiple functions contain commented-out assertions that originally performed input validation. Due to the resulting lack of validation, it is possible to cause the SyncSwap pool contracts to invoke another contract with incorrect parameters via the ICallback interface.

For instance, the burnSingle function of both SyncSwapStablePool and SyncSwapClassicPool might invoke the callback contract as well as return an unexpected value, since params.tokenOut is not constrained to be either token0 or token1.

function burnSingle(/* [...] */) external override nonReentrant returns (TokenAmount memory _tokenAmount) {
    ICallback.BaseBurnSingleCallbackParams memory params;
!   (params.tokenOut, params.to, params.withdrawMode) = 
!    abi.decode(_data, (address, address, uint8));
    // [...]
    if (_callback != address(0)) {
        // Fills additional values for callback params.
        params.sender = _sender;
        params.callbackData = _callbackData;
!       ICallback(_callback).syncSwapBaseBurnSingleCallback(params);
    }
    // [...]
!   _tokenAmount = TokenAmount(params.tokenOut, params.amountOut);
    emit Burn(msg.sender, params.amount0, params.amount1, params.liquidity, params.to);
}

This issue is also present in swap, which does not properly check params.tokenIn:

function swap(/* [...] */) external override nonReentrant returns (TokenAmount memory _tokenAmount) {
    ICallback.BaseSwapCallbackParams memory params;
!   (params.tokenIn, params.to, params.withdrawMode) = 
!   abi.decode(_data, (address, address, uint8));
    // [...]
    if (_callback != address(0)) {
        // Fills additional values for callback params.
        params.sender = _sender;
        params.callbackData = _callbackData;
!       ICallback(_callback).syncSwapBaseSwapCallback(params);
    }
    // Updates reserves with up-to-date balances (updated above).
    _updateReserves(params.balance0, params.balance1);
    _tokenAmount.token = params.tokenOut;
    _tokenAmount.amount = params.amountOut;
}

Impact

This does not directly impact the contracts in scope of this audit. However, it does allow an attacker to manipulate the pool contracts into invoking a third party contract with unexpected parameters. The consequences of this issue would depend on the implementation of the contract being invoked.

Recommendations

We generally recommend including conservative assertions validating all the inputs. If these checks are deemed too expensive, we suggest removing the commented-out code to improve readability and inserting a comment stating why such a check is not needed.

Since SyncSwap contracts cannot be upgraded, we recommend updating the documentation of the contracts to clearly communicate to users of the contract how a callback invocation can be verified. Implementors of the ICallback interface should carefully analyze how the potentially untrusted parameters could cause issues in their contracts.

In general, depending on the usage, the two following strategies should suffice to guard against exploitation:

• if the callback is only supposed to be invoked when the Pool contract is called by a trusted address, check that the sender field of the callback parameters is in the set of trusted addresses

• otherwise (or in addition), ensure that ((tokenIn == pool.token0() && tokenOut == pool.token1()) || (tokenIn == pool.token1() && tokenOut == pool.token0()))

Remediation

The development team has acknowledged this issue and added a notice warning about the untrusted nature of tokenIn and tokenOut in commit 5285a3a7↗.