Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Solana][NONEVM-1246] Implement allowlist on onramp #520

Merged
merged 6 commits into from
Jan 31, 2025
Merged

[Solana][NONEVM-1246] Implement allowlist on onramp #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a5f3e3
Implement allowlist on onramp
PabloMansanet Jan 29, 2025
8f697d5
Merge branch 'main' into onramp_allowlist
PabloMansanet Jan 31, 2025
e4d4172
Add clarifying comment
PabloMansanet Jan 31, 2025
1488282
Merge branch 'main' into onramp_allowlist
PabloMansanet Jan 31, 2025
6f46ba4
Merge branch 'main' into onramp_allowlist
PabloMansanet Jan 31, 2025
e81b98b
Merge branch 'main' into onramp_allowlist
aalu1418 Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions chains/solana/contracts/programs/ccip-router/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use solana_program::sysvar::instructions;
use crate::program::CcipRouter;
use crate::state::{CommitReport, Config, Nonce};
use crate::{
BillingTokenConfig, BillingTokenConfigWrapper, CcipRouterError, DestChain,
BillingTokenConfig, BillingTokenConfigWrapper, CcipRouterError, DestChain, DestChainConfig,
ExecutionReportSingleChain, ExternalExecutionConfig, GlobalState, SVM2AnyMessage, SourceChain,
};

Expand Down Expand Up @@ -254,11 +254,10 @@ pub struct AcceptOwnership<'info> {
}

#[derive(Accounts)]
#[instruction(new_chain_selector: u64)]
#[instruction(new_chain_selector: u64, dest_chain_config: DestChainConfig)]
pub struct AddChainSelector<'info> {
/// Adding a chain selector implies initializing the state for a new chain,
/// hence the need to initialize two accounts.

#[account(
init,
seeds = [seed::SOURCE_CHAIN_STATE, new_chain_selector.to_le_bytes().as_ref()],
Expand All @@ -273,7 +272,7 @@ pub struct AddChainSelector<'info> {
seeds = [seed::DEST_CHAIN_STATE, new_chain_selector.to_le_bytes().as_ref()],
bump,
payer = authority,
space = ANCHOR_DISCRIMINATOR + DestChain::INIT_SPACE,
space = ANCHOR_DISCRIMINATOR + DestChain::INIT_SPACE + dest_chain_config.dynamic_space(),
)]
pub dest_chain_state: Account<'info, DestChain>,

Expand Down Expand Up @@ -312,8 +311,37 @@ pub struct UpdateSourceChainSelectorConfig<'info> {
}

#[derive(Accounts)]
#[instruction(new_chain_selector: u64)]
#[instruction(new_chain_selector: u64, dest_chain_config: DestChainConfig)]
pub struct UpdateDestChainSelectorConfig<'info> {
#[account(
mut,
seeds = [seed::DEST_CHAIN_STATE, new_chain_selector.to_le_bytes().as_ref()],
bump,
constraint = valid_version(dest_chain_state.version, MAX_CHAINSTATE_V) @ CcipRouterError::InvalidInputs,
realloc = ANCHOR_DISCRIMINATOR + DestChain::INIT_SPACE + dest_chain_config.dynamic_space(),
realloc::payer = authority,
// `realloc::zero = true` is only necessary in cases where an instruction is capable of reallocating
// *down* and then *up*, during a single execution. In any other cases (such as this), it's not
// necessary as the memory will be zero'd automatically on instruction entry.
realloc::zero = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using false, then we need to ensure zeroing the bytes when decreasing the size of the allowed senders?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Do we? I'm not sure.

The logic is pretty simple and self-contained, so I'm not sure what damage leaving those bytes "dirty" could cause. If we downsize and upsize afterwards, all code paths lead to overwriting these bytes again, so I can't see how we'd end up with garbage in an allocated structure.

I might be missing some solana risks I'm not aware of here though, so if it's safer to make this true, I'm happy to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the docs that I read recommend using realloc zero to true, or in this doc they recommend using an ALT table instead

https://www.helius.dev/blog/a-hitchhikers-guide-to-solana-program-security?utm_source=chatgpt.com#account-data-reallocation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module talks about issues when init, but they may be valid for resize as well? https://solana.com/developers/courses/program-security/reinitialization-attacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agusaldasoro the guide you linked (and the official documentation) bot specify that realloc::zero = true is only necessary in the cases where a single instruction can downsize and then upsize in one call. This is impossible in our case as we only reallocate once on entry. This means the bytes are automatically zeroed, and using realloc::zero = true would be wasted cycles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, okay! that's clear now. Can you please add a comment in code staying that so we don't forget about that in the future? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! :)

)]
pub dest_chain_state: Account<'info, DestChain>,

#[account(
seeds = [seed::CONFIG],
bump,
constraint = valid_version(config.load()?.version, MAX_CONFIG_V) @ CcipRouterError::InvalidInputs,
)]
pub config: AccountLoader<'info, Config>,

#[account(mut, address = config.load()?.owner @ CcipRouterError::Unauthorized)]
pub authority: Signer<'info>,
pub system_program: Program<'info, System>,
}

#[derive(Accounts)]
#[instruction(new_chain_selector: u64)]
pub struct DisableDestChainSelectorConfig<'info> {
#[account(
mut,
seeds = [seed::DEST_CHAIN_STATE, new_chain_selector.to_le_bytes().as_ref()],
Expand Down
4 changes: 2 additions & 2 deletions chains/solana/contracts/programs/ccip-router/src/instructions/v1/admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anchor_lang::prelude::*;
use anchor_spl::token_interface;

use crate::seed;
use crate::{seed, DisableDestChainSelectorConfig};
use crate::{
AcceptOwnership, AddBillingTokenConfig, AddChainSelector, BillingTokenConfig, CcipRouterError,
DestChainAdded, DestChainConfig, DestChainConfigUpdated, DestChainState, FeeTokenAdded,
Expand Down Expand Up @@ -107,7 +107,7 @@ pub fn disable_source_chain_selector(
}

pub fn disable_dest_chain_selector(
ctx: Context<UpdateDestChainSelectorConfig>,
ctx: Context<DisableDestChainSelectorConfig>,
dest_chain_selector: u64,
) -> Result<()> {
let chain_state = &mut ctx.accounts.dest_chain_state;
Expand Down
2 changes: 2 additions & 0 deletions chains/solana/contracts/programs/ccip-router/src/instructions/v1/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,8 @@ pub mod ramps {
gas_price_staleness_threshold: 90000,
enforce_out_of_order: false,
chain_family_selector: CHAIN_FAMILY_SELECTOR_EVM.to_be_bytes(),
allowed_senders: vec![],
allow_list_enabled: false,
},
}
}
Expand Down
8 changes: 7 additions & 1 deletion chains/solana/contracts/programs/ccip-router/src/instructions/v1/onramp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,15 @@ pub fn ccip_send<'info>(
// The Config Account stores the default values for the Router, the SVM Chain Selector, the Default Gas Limit and the Default Allow Out Of Order Execution and Admin Ownership
let config = ctx.accounts.config.load()?;

let sender = ctx.accounts.authority.key.to_owned();
let dest_chain = &mut ctx.accounts.dest_chain_state;

require!(
!dest_chain.config.allow_list_enabled
|| dest_chain.config.allowed_senders.contains(&sender),
CcipRouterError::SenderNotAllowed
);

let mut accounts_per_sent_token: Vec<TokenAccounts> = vec![];

for (i, token_amount) in message.token_amounts.iter().enumerate() {
Expand Down Expand Up @@ -163,7 +170,6 @@ pub fn ccip_send<'info>(
);
dest_chain.state.sequence_number = overflow_add.unwrap();

let sender = ctx.accounts.authority.key.to_owned();
let receiver = message.receiver.clone();
let source_chain_selector = config.svm_chain_selector;
let nonce_counter_account: &mut Account<'info, Nonce> = &mut ctx.accounts.nonce;
Expand Down
4 changes: 3 additions & 1 deletion chains/solana/contracts/programs/ccip-router/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ pub mod ccip_router {
/// * `ctx` - The context containing the accounts required for disabling the chain selector.
/// * `dest_chain_selector` - The destination chain selector to be disabled.
pub fn disable_dest_chain_selector(
ctx: Context<UpdateDestChainSelectorConfig>,
ctx: Context<DisableDestChainSelectorConfig>,
dest_chain_selector: u64,
) -> Result<()> {
v1::admin::disable_dest_chain_selector(ctx, dest_chain_selector)
Expand Down Expand Up @@ -660,4 +660,6 @@ pub enum CcipRouterError {
InvalidTokenReceiver,
#[msg("Invalid SVM address")]
InvalidSVMAddress,
#[msg("Sender not allowed for that destination chain")]
SenderNotAllowed,
}
19 changes: 19 additions & 0 deletions chains/solana/contracts/programs/ccip-router/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,25 @@ pub struct DestChainConfig {
pub gas_price_staleness_threshold: u32, // The amount of time a gas price can be stale before it is considered invalid (0 means disabled)
pub enforce_out_of_order: bool, // Whether to enforce the allowOutOfOrderExecution extraArg value to be true.
pub chain_family_selector: [u8; 4], // Selector that identifies the destination chain's family. Used to determine the correct validations to perform for the dest chain.

// list of senders authorized to send messages to this destination chain.
// Note: The attribute name `max_len` is slightly misleading: it is not in any
// way limiting the actual length of the vector during initialization; it just
// helps the InitSpace derive macro work out the initial space. We can leave it at
// zero and calculate the actual length in the instruction context.
#[max_len(0)]
pub allowed_senders: Vec<Pubkey>,
pub allow_list_enabled: bool,
Comment on lines +123 to +125
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we double check the product requirements for allowed_senders?

if it's unbounded, then we should allowlist based on a user specific PDA since there is a maximum account size that would bound the list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked by @agusaldasoro , we're OK to go with the boundary enforced by maximum account size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We estimated a max size of 300k which is okay

}

impl DestChainConfig {
pub fn space(&self) -> usize {
Self::INIT_SPACE + self.dynamic_space()
}

pub fn dynamic_space(&self) -> usize {
self.allowed_senders.len() * std::mem::size_of::<Pubkey>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't arrays have a base size of 4? I remember using 4 + len * size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is captured by INIT_SPACE. When specifying a max_len of 0, INIT_SPACE only considers the vector discriminator (4 bytes), so we only need to provide the actual dynamic space in this method.

I find it a bit annoying that 'max_len' is so strangely named, because it's way more useful than I first thought: It should be called elements_to_preallocate_at_init or something 😆.

}
}

#[account]
Expand Down
18 changes: 18 additions & 0 deletions chains/solana/contracts/target/idl/ccip_router.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,11 @@
"name": "authority",
"isMut": true,
"isSigner": true
},
{
"name": "systemProgram",
"isMut": false,
"isSigner": false
}
],
"args": [
Expand Down Expand Up @@ -2486,6 +2491,16 @@
4
]
}
},
{
"name": "allowedSenders",
"type": {
"vec": "publicKey"
}
},
{
"name": "allowListEnabled",
"type": "bool"
}
]
}
Expand Down Expand Up @@ -2773,6 +2788,9 @@
},
{
"name": "InvalidSVMAddress"
},
{
"name": "SenderNotAllowed"
}
]
}
Expand Down
146 changes: 146 additions & 0 deletions chains/solana/contracts/tests/ccip/ccip_router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,7 @@ func TestCCIPRouter(t *testing.T) {
destChainStatePDA,
config.RouterConfigPDA,
admin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result := testutils.SendAndFailWith(ctx, t, solanaGoClient, []solana.Instruction{instruction}, admin, config.DefaultCommitment, []string{"Error Code: " + ccip_router.InvalidInputs_CcipRouterError.String()})
Expand Down Expand Up @@ -702,6 +703,7 @@ func TestCCIPRouter(t *testing.T) {
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
user.PublicKey(), // unauthorized
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result := testutils.SendAndFailWith(ctx, t, solanaGoClient, []solana.Instruction{instruction}, user, config.DefaultCommitment, []string{"Error Code: " + ccip_router.Unauthorized_CcipRouterError.String()})
Expand Down Expand Up @@ -751,6 +753,7 @@ func TestCCIPRouter(t *testing.T) {
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
admin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result := testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{instruction}, admin, config.DefaultCommitment)
Expand Down Expand Up @@ -2092,6 +2095,72 @@ func TestCCIPRouter(t *testing.T) {
require.NotNil(t, result)
})

t.Run("When sending with an empty but enabled allowlist, it fails", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test for sending with disabled allowlist as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary: that's already captured by all the other tests, as the allowlist is disabled by default.

var initialDestChain ccip_router.DestChain
err := common.GetAccountDataBorshInto(ctx, solanaGoClient, config.EvmDestChainStatePDA, config.DefaultCommitment, &initialDestChain)
require.NoError(t, err, "failed to get account info")
modifiedDestChain := initialDestChain
modifiedDestChain.Config.AllowListEnabled = true

updateDestChainIx, err := ccip_router.NewUpdateDestChainConfigInstruction(
config.EvmChainSelector,
modifiedDestChain.Config,
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
anotherAdmin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result := testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{updateDestChainIx}, anotherAdmin, config.DefaultCommitment)
require.NotNil(t, result)

destinationChainSelector := config.EvmChainSelector
destinationChainStatePDA := config.EvmDestChainStatePDA
message := ccip_router.SVM2AnyMessage{
FeeToken: wsol.mint,
Receiver: validReceiverAddress[:],
Data: []byte{4, 5, 6},
ExtraArgs: emptyEVMExtraArgsV2,
}

raw := ccip_router.NewCcipSendInstruction(
destinationChainSelector,
message,
[]byte{},
config.RouterConfigPDA,
destinationChainStatePDA,
nonceEvmPDA,
user.PublicKey(),
solana.SystemProgramID,
wsol.program,
wsol.mint,
wsol.billingConfigPDA,
token2022.billingConfigPDA,
wsol.userATA,
wsol.billingATA,
config.BillingSignerPDA,
config.ExternalTokenPoolsSignerPDA,
)
raw.GetFeeTokenUserAssociatedAccountAccount().WRITE()
instruction, err := raw.ValidateAndBuild()
require.NoError(t, err)
result = testutils.SendAndFailWith(ctx, t, solanaGoClient, []solana.Instruction{instruction}, user, config.DefaultCommitment, []string{"Error Code: SenderNotAllowed"})
require.NotNil(t, result)

// We now restore the config to keep the test state-neutral
updateDestChainIx, err = ccip_router.NewUpdateDestChainConfigInstruction(
config.EvmChainSelector,
initialDestChain.Config,
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
anotherAdmin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result = testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{updateDestChainIx}, anotherAdmin, config.DefaultCommitment)
require.NotNil(t, result)
})

t.Run("When sending a Valid CCIP Message Emits CCIPMessageSent", func(t *testing.T) {
destinationChainSelector := config.EvmChainSelector
destinationChainStatePDA := config.EvmDestChainStatePDA
Expand Down Expand Up @@ -2851,6 +2920,83 @@ func TestCCIPRouter(t *testing.T) {
})
})

t.Run("When sending with an enabled allowlist including the sender, it succeeds", func(t *testing.T) {
var initialDestChain ccip_router.DestChain
err := common.GetAccountDataBorshInto(ctx, solanaGoClient, config.EvmDestChainStatePDA, config.DefaultCommitment, &initialDestChain)
require.NoError(t, err, "failed to get account info")
modifiedDestChain := initialDestChain
modifiedDestChain.Config.AllowListEnabled = true
modifiedDestChain.Config.AllowedSenders = []solana.PublicKey{
user.PublicKey(),
anotherUser.PublicKey(),
}

updateDestChainIx, err := ccip_router.NewUpdateDestChainConfigInstruction(
config.EvmChainSelector,
modifiedDestChain.Config,
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
anotherAdmin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result := testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{updateDestChainIx}, anotherAdmin, config.DefaultCommitment)
require.NotNil(t, result)

var parsedDestChain ccip_router.DestChain
err = common.GetAccountDataBorshInto(ctx, solanaGoClient, config.EvmDestChainStatePDA, config.DefaultCommitment, &parsedDestChain)
require.NoError(t, err, "failed to get account info")

// This proves we're able to update the config with a dynamically sized element
require.Equal(t, parsedDestChain.Config.AllowedSenders, modifiedDestChain.Config.AllowedSenders)

destinationChainSelector := config.EvmChainSelector
destinationChainStatePDA := config.EvmDestChainStatePDA
message := ccip_router.SVM2AnyMessage{
FeeToken: wsol.mint,
Receiver: validReceiverAddress[:],
Data: []byte{4, 5, 6},
ExtraArgs: emptyEVMExtraArgsV2,
}

raw := ccip_router.NewCcipSendInstruction(
destinationChainSelector,
message,
[]byte{},
config.RouterConfigPDA,
destinationChainStatePDA,
nonceEvmPDA,
user.PublicKey(),
solana.SystemProgramID,
wsol.program,
wsol.mint,
wsol.billingConfigPDA,
token2022.billingConfigPDA,
wsol.userATA,
wsol.billingATA,
config.BillingSignerPDA,
config.ExternalTokenPoolsSignerPDA,
)
raw.GetFeeTokenUserAssociatedAccountAccount().WRITE()
instruction, err := raw.ValidateAndBuild()
require.NoError(t, err)
result = testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{instruction}, user, config.DefaultCommitment)
require.NotNil(t, result)

// We now restore the config to keep the test state-neutral
updateDestChainIx, err = ccip_router.NewUpdateDestChainConfigInstruction(
config.EvmChainSelector,
initialDestChain.Config,
config.EvmDestChainStatePDA,
config.RouterConfigPDA,
anotherAdmin.PublicKey(),
solana.SystemProgramID,
).ValidateAndBuild()
require.NoError(t, err)
result = testutils.SendAndConfirm(ctx, t, solanaGoClient, []solana.Instruction{updateDestChainIx}, anotherAdmin, config.DefaultCommitment)
require.NotNil(t, result)
})

t.Run("token pool accounts: validation", func(t *testing.T) {
t.Parallel()
// base transaction
Expand Down
Loading
Loading