Coinbase: solana-stable-swapper

Description

The original initialize() allowed the default address for the fee recipient.

The new initialize() checks for this via require_recipient_set().

Recommendation

Consider checking this in do_migrate_authorities().

Description

A test for Unpause uses pauseAuthority, which looks to be incorrect.

Recommendation

Consider changing the test to:

.accounts({    pool,-   pauseAuthority: pauseAuthority.publicKey,+   unpauseAuthority: unpauseAuthority.publicKey,  })-  .signers([pauseAuthority.payer])+  .signers([unpauseAuthority.payer])

Description

When update_fee_recipient is called, only the pubkey is updated in the pool state. No associated token accounts are created for the new fee recipient. During subsequent swaps, the fee_recipient_token_account uses init_if_needed with the swapping user as payer. This means the first user to swap each supported token after a fee recipient change will pay approximately 0.002 SOL to create the new recipients ATA.

In contrast, when adding a new token via add_supported_token, the configure_authority pays for the fee recipients ATA creation.

Recommendation

Consider requiring the configure_authority to pre create all necessary ATAs for the new fee recipient within the update_fee_recipient instruction. This would align the cost model with add_supported_token and shift the burden to the party making the configuration change.

Coinbase

Good point, but this is out of scope for this review.

Description

The Solana swap instruction validates that amount_in is greater than zero and that the computed amount_out is greater than zero, but it never requires min_amount_out to be greater than zero. Because the only slippage guard is amount_out >= min_amount_out, a caller who passes min_amount_out equal to zero reduces that guard to a comparison that always succeeds, so slippage protection is effectively turned off for that swap.

The EVM implementation does not permit this. Its swap function explicitly rejects a zero minimum, forcing every caller to commit to a real minimum output before the swap executes. The relevant EVM guard is on L344 in function swap().

This is a behavioral divergence between the two chains for the same product. The two implementations are meant to mirror each other, yet the Solana side accepts an input that the EVM side treats as invalid. This is a small parity and hardening note to further tighten the codebase.

Recommendation

Add an explicit check that min_amount_out is greater than zero at the start of the swap instruction, mirroring the EVM CannotBeZeroAmount guard so both chains enforce the same invariant:

require!(amount_in > 0, LiquidityError::InvalidAmount);+ require!(min_amount_out > 0, LiquidityError::InvalidAmount);

This forces every caller to specify a real minimum output and removes the ability to submit a swap with slippage protection disabled, bringing the Solana behavior back in line with the EVM contract.

Description

The hot key comment is not accurate because there is a list of allowed withdraw_recipients.

Recommendation

Consider changing the comment in the following way:

- /// Hot key allowed to withdraw liquidity (only to `withdraw_recipient`).+ /// Hot key allowed to withdraw liquidity (only to one of the `withdraw_recipients`).

Description

The variable MIGRATION_GROWTH is only used in comments.

Recommendation

Consider removing the variable or turning the line into a comment.

Description

Function initialize() uses .key() while migrate_authorities() and most other functions uses Pubkey. The result is the same but its inconsistent.

Recommendation

Consider using the same convention everywhere. However because it changes the interface is might not be worth the trouble.

Description

The initial state of a newly added token is different between EVM and Solana. On the EVM the initial state is that swappable=false and on Solana the initial state is vault.disabled = false;, which means it is swappable.

Both versions are meant to have the same functionality.

Recommendation

Consider using the same approach on both versions.

Description

“Liquidity paused” means withdrawals paused, however swaps and direct deposits are still allowed. So the naming "LiquidityPaused" and the error message is confusing.

Recommendation

Consider renaming "LiquidityPaused" to "Withdrawel paused", both the error string and the variable name.

Description

Some comments in stable-swapper.ts are obsolete and confusing.

Recommendation

Consider changing the comments in the following way, or remove the comments.

- .pauseWithdraws() // swapsPaused=null, liquidityPaused=true+ .pauseWithdraws() // Sets liquidityPaused=true; swapsPaused is unchanged- .pauseSwaps() // swapsPaused, liquidityPaused+ .pauseSwaps() // Sets swapsPaused=true; liquidityPaused is unchanged- .unpauseSwaps() // swapsPaused, liquidityPaused+ .unpauseSwaps() // Sets swapsPaused=false; liquidityPaused is unchanged