Blazing Trading Suite

In general, the Blazing Trading Suite protocol allows users to interact with arbitrary pools. Some of these pools can have malicious behaviours and transferring control to them can expose the protocol to reentrancy attacks. Although no specific attack was identified, we suggest whitelisting the pools which the protocol is allowed to interact with.

Dispatcher::permit2PermitDispatch accepts a failed call to PERMIT2.permit if a valid allowance is already set, in order to prevent DoS scenarios in which an adversary intercepts the signature and calls permit himself in order to make the "duplicate" permit fail. Although we understand the motivation, and this behaviour does not by itself create an immediate threat, this is still not a safe practice and could potentially facilitate an attack scenario in combination with other vulnerabilities. While a valid permit included in a transaction shows the intention to spend the corresponding funds, if the permit is invalid, the user's intention is much less clear. The contract will even accept permits with wrong signatures (not just duplicates), and tolerating wrong signatures makes it easier to exploit vulnerabilities.

We recommend limiting the accepted errors as much as possible. For instance, the contract could accept only InvalidNonce() errors from Permit2, which indicate a possible replay attack, but _not_ incorrect signatures. Moreover, the contract could check that the expiration of the existing allowance is exactly that of the submitted permit, not just greater or equal, again trying to accept duplicate permits while minimising other accepted errors.

The contract's balance is used to store two unrelated things: taxes and user funds for the currently executed swaps. This mix could potentially lead to a loss of funds if an accounting bug is found, either in the current code or in future iterations. Although we did not identify accounting issues, the risk would be substantially lowered by keeping taxes in a separate contract that never receives user funds.

BlazingRouter::execute considers any funds which are owned by the contract and which are not fees as belonging to the user, even if the funds were not obtained in the current transaction.

This can allow an adversary to artificially inflate the contract's balance at any moment, by transferring an arbitrary amount of native token to the contract (via selfdestruct), knowing that he can recover these funds in the next transaction.

Although we did not identify a specific attack, the possibility of arbitrarily inflating the contract's balance is commonly exploited in combination with other vulnerabilities, hence it would be preferable to avoid it.

An alternative could be to consider that only funds obtained in the current transaction belong to the user, and keep the remaining funds in the contract, making it financially difficult for the adversary to inflate the balance.

The BlazingRouterV21 execute function reverts if currentBalance < newProtocolTaxesBalance. If there is any bug in the protocol, the execute function will freeze and brick the entire protocol. This also affects the BlazingRouterV22, BlazingRouterV31 and BlazingRouterV32 contracts.

The contracts use the UUPS pattern but do not use “gaps” to leave space for future state variables. As a consequence, adding a state variable to any base contract will break the state layout after the upgrade.

The /helper/modules/payments/Payments contract has a disperseChainCurrency function which loops over a recipients array to disperse the chain currency to users. This function is called by the /helper/base/Dispatcher::disperseChainCurrencyDispatch function which indicates that it is supposed to be able to handle a maximum of 838_860 recipients. We advise metering this function to ensure that the disperseChainCurrency function does not run out of gas over such a large number of recipients before deployment. The same issue affects Payments::disperseToken.

The BlazingRouterV21 contract inherits from Initializable and OwnableUpgradable. However, its parent /v2/base/LockAndMsgSender already inherits from Initializable, and its parent /v2/base/ProtocolTaxesManager already inherits from Initializable and from OwnableUpgradable. This issue also affects BlazingRouterV22, BlazingRouterV31 and BlazingRouterV32 (the latter two relative to the /v3/base contracts).

The /v2/base/Dispatcher inherits from Payments, but its parent /v2/modules/payments/Permit2Payments already inherits from the /v2/modules/payments/Payments contract. Also affects /v3/base/Dispatcher.

The /v2/modules/Protection contract has a checkHoneyPot function which reverts if the interaction with the pool fails with a reason which is not 64 bytes long. However this exposes the users to honeypots with a reason which is 64 bytes long.

The /v2/modules/Protection contract has a function called checkLiquidity which checks that the final pool on a swap path has liquidity between certain lower and upper bounds. However, it does not check that the intermediate pools also have a certain liquidity, which may be misleading to the caller of the function. Also affects /v3/modules/Protection.

Although not enforced, the comments in Dispatcher::permit2PermitDispatch suggest that expiration = 0 will always be used. However, this does not necessarily increase security; the permit will be valid for just the current block, but it will be accepted in _any_ block. In a hypothetical scenario in which the adversary wants to use the permit in some attack, if the transaction is not executed (so the permit

remains valid), it will stay valid forever and the adversary can submit it himself at any moment, the only constraint being that he will need to execute his attack in the same block (a quite soft constraint).

In some scenarios, for instance transactions with a forced block or known deadline, it might be actually preferable to set the expiration to that specific block, severely limiting the lifespan of the permit.

The /helper/modules/payments/Payments contract has two functions disperseChainCurrency and disperseToken which should check whether the recipients and the values array have the same length if the useSameAmount flag is false.

The /v2/base/ProtocolTaxesManager function is an onlyOwner setter function, but does not have any validations on the tax amounts being set. Also affects /v3/base/ProtocolTaxesManager.

The /v3/libraries/BytesLib toLastPool function assigns a hex value to the offsetValue variable and a decimal value to the lastPoolOffsetSub variable. We recommend choosing one representation for consistency.

The code is compiled with Solidity ^0.8.20. For deployment, we recommend no floating pragmas, but a specific version, to be confident about the baseline guarantees offered by the compiler. Version 0.8.20, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.