[Solana][NONEVM-1246] Implement allowlist on onramp #520
Conversation
| #[max_len(0)] | ||
| pub allowed_senders: Vec<Pubkey>, | ||
| pub allow_list_enabled: bool, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Checked by @agusaldasoro , we're OK to go with the boundary enforced by maximum account size.
There was a problem hiding this comment.
We estimated a max size of 300k which is okay
| } | ||
|
|
||
| pub fn dynamic_space(&self) -> usize { | ||
| self.allowed_senders.len() * std::mem::size_of::<Pubkey>() |
There was a problem hiding this comment.
Don't arrays have a base size of 4? I remember using 4 + len * size
There was a problem hiding this comment.
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 😆.
| @@ -2138,6 +2141,71 @@ 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) { | |||
There was a problem hiding this comment.
Should we add a test for sending with disabled allowlist as well?
There was a problem hiding this comment.
I don't think it's necessary: that's already captured by all the other tests, as the allowlist is disabled by default.
| 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 = false |
There was a problem hiding this comment.
If using false, then we need to ensure zeroing the bytes when decreasing the size of the allowed senders?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
All the docs that I read recommend using realloc zero to true, or in this doc they recommend using an ALT table instead
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Sounds good! :)
PabloMansanet
force-pushed
the
onramp_allowlist
branch
from
a99f0b1 to
3a5f3e3
Compare
|
No description provided.