1inch Fusion Solana Implementation Security Review Report

The Fusion Swap program allows a user to create an order and cancel an order permissionlessly, containing some set of configurable parameters. A taker would have to accept the order by submitting a transaction calling fill, which requires the taker to be whitelisted.

Nevertheless, it is possible to steal from the taker directly by front-running the order fulfillment and swapping out the order's data by reusing the order ID.

The order is stored at a PDA that is calculated from a constant seed, the maker's address and the order ID. If the maker cancels their order, the escrow PDA is completely closed (as it should), but this allows for reinitialization and reuse of the same order ID and escrow address.

This means that a transaction containing fill and the escrow account from the initial order, could be forced to fill a second malicious order that uses the same order ID.

For example, the attacker can swap out:

• The rate at which the token is sold, making the taker pay more.
• Increase the fees, making the taker pay more.
• Change the tokens to be native using the bool switch, which will cause the provided token to be ignored and the taker's SOL balance to be used instead of for example a USD stable coin.

The order ID should not be reusable for the maker, such that the same PDA can never be derived twice. This can be implemented using a new state account that keeps a bitmap of maker to order IDs.

Another layer of protection could be a slippage control for the taker to specify the maximum amount of tokens to be spent.

According to the documentation, the create function is used for creating orders with special parameters by the maker:

• Context<Create> bundles all the required accounts, including the escrow account to be created, the maker's token accounts, etc.
• order: OrderConfig is the struct that holds important parameters for this new order. Within this function, there is a particular validation for order.expiration_time, which is intended to prevent creating an order that is already expired at the time of creation:

//programs/fusion-swap/src/lib.rs#L40-L43
 
...
 
require!(
    Clock::get()?.unix_timestamp <= order.expiration_time as i64,
    EscrowError::OrderExpired
);
 
...

There is a special validation check of input regarding order.experation which is designed to prevent creating an order that is already expired at the time of creation. But this check is incomplete because technically maker can create an order in which experation_time equals unix_timestamp, and this order will not have the opportunity to be filled.

This could lead to a situation when the taker will spend gas on a transaction destined to fail due to the order being expired the moment it's created, leaving no window for a successful fill.

Commentary from the client:

The expiration buffer should be set based on the order's purpose by the client or the backend using this program. We ensure that the order is valid at the time of creation and we do not want to impose a minimum order lifespan.

Consider adding a minimum buffer for expiration_time during order creation.

The associated token account (ATA) for collecting protocol fees protocol_dst_ata is not verified/checked against a configured account. This means that a maker can set the protocol_dst_ata to be any arbitrary ATA when creating an order. As a result the protocol fee may be sent to an arbitrary ATA when the order is filled.

Note that according to the specification, the Surplus fee should be allocated to the DAO:

"The Surplus Fee applies to trades executed at a rate significantly higher than the current market rate. A portion of this excess value is allocated to the DAO to support protocol operations. And the remaining part of the excess goes to a user."

However, since the Surplus fee is added to the protocol fee and sent to the protocol_dst_ata the surplus fee might not be allocated to the DAO.

Commentary from the client:

Noted. We validate orders that signed through our apps on the backend before sharing them for filling.

Consider checking the protocol_dst_ata against a configured ATA.

fusion-swap does not prevent the creation of orders with identical maker and taker assets. There is unclear economic purpose for allowing trades between the same asset, and allowing this may encourage:

• Potential account derivation issues: allowing the same srcMint and dstMint assets means that mutable accounts used in the same fill instruction can be derived to the same address, such as escrow_src_ata and maker_dst_ata . Given that maker accounts are not checked, this allows for unfavorable situations where orders can be created with escrow.receiver being equal to the escrow's own ATA account.
• Fee farming: Any proposed means of providing rewards based on fees may be gamed by artificially increasing trading volume via same-asset trades.
• Liquidity lock: locking the same maker and taker assets in escrow accounts reduces the available liquidity for other trades involving the affected assets.

Consider preventing order creation if the srcMint and dstMint accounts are identical, with an appropriate custom error.

//programs/fusion-swap/src/lib.rs#L28-L104
...
pub fn create ctx order Context Create OrderConfig Result
( : < >, : ) -> <()> {
...
++ require!(
++ ctx.accounts.src_mint.key() != ctx.accounts.dst_mint.key(),
++ EscrowError::SameAsset
++ );
...
...

fusion-swap does not prevent the fulfillment of orders with identical maker and taker accounts. Although this was not observed to be directly exploitable, this allows for self-fulfillment of orders for whitelisted taker accounts. This will allow for the easier exploitation of the following issues, including:

• Artificial trading volume inflation: via repeatedly trading between the same account, as there would be no need to pay for additional transaction signing fees for separate maker/taker accounts.
• Reward farming: as arbitrary protocol_dst_ata and integrator_dst_ata accounts can be specified, this may allow for exploitation of any currently or future-planned reward distribution mechanisms. Additionally, due to Solana's account derivation model, allowing the same maker/taker increases the chances of future insecure seed derivation findings for accounts using seeds derived from either maker or taker accounts, as maker and taker accounts are used to deserialize key PDAs such as escrow and taker_dst_ata.

The effects of this are increased given that there is also no restriction preventing the same asset from being used as both the maker and taker mint accounts as outlined in finding OIN8-9.

If self-trading between the same account is not intended, consider preventing order fulfillment for a given escrow if the taker account is the same as the maker, with a specific custom error:

//programs/fusion-swap/src/lib.rs#L28-L104
...
pub fn fill ctx order_id amount Context Fill u32 u64
( : < >, : , : ) ->
Result
<()> {
...
++ require!(
++ ctx.accounts.maker.key() != ctx.accounts.taker.key(),
++ EscrowError::MakerAndTakerIdentical
);
...
}
...

There are multiple TODO's relating to account derivation logic in programs/fusion-swap/src/lib.rs.

//programs/fusion-swap/src/lib.rs#L374,L421,L456
 
...
// TODO: Add src_mint to escrow or seeds
...
// TODO initialize this account as well as 'maker_dst_ata'
// this needs providing receiver address and adding
// associated_token::mint = dst_mint,
// associated_token::authority = receiver
// constraint
...
// TODO: Add src_mint to escrow or seeds
...

Consider resolving the TODO's.

Single-step ownership transfer to non-Signer may introduce a risk of setting an unwanted owner by accident if the ownership transfer is not done with excessive care.

The _new_owner argument of the transfer_ownership() function is of type Pubkey and thus may introduce the risk of transferring the ownership to an unowned address.

//programs/whitelist/src/lib.rs#L35-L40
...
/// Transfers ownership of the whitelist to a new owner
pub fn
transfer_ownership
( : < >, :
ctx _new_owner
Context TransferOwnership
Pubkey Result
) -> <()> {
let mut
whitelist_state ctx
= & .accounts.whitelist_state;
whitelist_state _new_owner
.owner = . ();
key
Ok
(())
}
...

Consider specifying the new owner as a Signer type inside the TransferOwnership struct. Or implement some 2-step ownership transfer mechanism.

The functions in the contract (e.g., register, deregister, transfer_ownership) lack event emissions for important actions like user registration, removal, and ownership transfer. Adding events to these functions would provide better transparency and allow for off-chain tracking of contract activity.

//programs/whitelist/src/lib.rs#L24-L39
...
/// Registers a new user to the whitelist
pub fn
register
( : < >, : ) -> <()> {
_ctx _user
Context Register Pubkey Result
Ok
(())
}
/// Removes a user from the whitelist
pub fn
deregister
( : < >, : ) ->
_ctx _user
Context Deregister Pubkey
Result
<()> {
Ok
(())
}
/// Transfers ownership of the whitelist to a new owner
pub fn
transfer_ownership
( : < >, :
ctx _new_owner
Context TransferOwnership
Pubkey Result
) -> <()> {
let mut
whitelist_state ctx
= & .accounts.whitelist_state;
whitelist_state _new_owner
.owner = . ();
key
Ok
(())
}
...

Commentary from the client:

Noted. Save money because it's possible to parse txs.

DESCRIPTION: An inaccurate error of ProgramError::ArithmeticOverflow will be thrown when the actual_dst_amount calculation underflows during the get_fee_amounts instruction. This may complicate debugging considering several other checks also throw ProgramError::ArithmeticOverflow upon genuine overflows.

//programs/fusion-swap/src/lib.rs#625
 
...
 
let actual_dst_amount = (dst_amount - protocol_fee_amount)
    .checked_sub(integrator_fee_amount)
    .ok_or(ProgramError::ArithmeticOverflow)?;
 
...

Consider implementing a custom UnderflowError to differentiate genuine underflow errors.

Commentary from the client:

Solana does not provide a distinct error for underflow. Since ProgramError::ArithmeticOverflow is the only native arithmetic-related error, we decided to use it for both overflow and underflow. We do not consider creating a separate custom error necessary.

The comment in the Deregister struct is misleading. It currently states: "Ensures only the whitelist owner can register new users," which is incorrect because the struct is responsible for removing a user from the whitelist, not registering them.

Update the comment to reflect the correct constraint.

The integrator_fee and protocol_fee values passed to the create instruction for the fusion-swap program are only checked for under/overflows during fee fulfillment, and not fee creation. This allows for orders to be created with excessive fee values which can never be fulfilled, as get_fee_amounts will underflow.

Consider using get_fee_amounts to check for fee validity during order creation in addition to order fulfillment to prevent stuck orders due to incompatible fee values.

Commentary from the client:

Noted. We prefer not to increase compute units and expect order creators to avoid setting invalid fees.