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.

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: emit -> emit_cpi #187

Merged
merged 2 commits into from
Oct 7, 2024
Merged

solana: emit -> emit_cpi #187

merged 2 commits into from
Oct 7, 2024

Conversation

a5-pickle
Copy link
Contributor

@a5-pickle a5-pickle commented Jul 10, 2024

Instead of using emit! by itself, we use emit_cpi! for all events. Only the AuctionUpdated event (emitted during the auction offer process) is written to the program logs in addition to event CPI.

This PR also adds an event listener in the Matching Engine SDK (onEventCpi) for the events that no longer log anything in program logs.

This is a breaking change. This PR modifies some event schemas to give more useful information. The Matching Engine SDK takes into account these schema changes. The following event schemas changed:

pub struct AuctionSettled {
    pub fast_vaa_hash: [u8; 32], // used to be `auction: Pubkey`
}

pub struct AuctionUpdated {
    pub config_id: u32,
    pub fast_vaa_hash: [u8; 32], // used to be `auction: Pubkey`
    ..
}

pub struct FastFillRedeemed {
    pub prepared_by: Pubkey,
    pub fast_fill: FastFillSeeds, // used to be `fast_fill: Pubkey`
}

pub struct OrderExecuted {
    pub fast_vaa_hash: [u8; 32], // used to be `auction: Pubkey`
    ..
}

Copy link

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

There's one instance that should still be changed IMO:

solana/programs/matching-engine/src/processor/fast_fill/complete.rs
82-
83-    // Emit event that the fast fill is redeemed. Listeners can close this account.
84:    emit!(crate::events::FastFillRedeemed {
85-        prepared_by: ctx.accounts.fast_fill.info.prepared_by,
86-        fast_fill: ctx.accounts.fast_fill.key(),

In addition there are several places in the codebase using msg! for logging. These case seem mostly to be outside of the general auction flow, and so might not have a security impact.

msg!() is mostly used in the context of executing admin actions. In this case a malicious admin could try to hide these artifacts. That said there is also a non-logging version of this function so they could just use this instead. In any case, there are other means for detecting an upgrade has changed.

So I think that this fix looks good so long as the last emit!() is addressed.

@a5-pickle
Copy link
Contributor Author

There's one instance that should still be changed IMO:

solana/programs/matching-engine/src/processor/fast_fill/complete.rs
82-
83-    // Emit event that the fast fill is redeemed. Listeners can close this account.
84:    emit!(crate::events::FastFillRedeemed {
85-        prepared_by: ctx.accounts.fast_fill.info.prepared_by,
86-        fast_fill: ctx.accounts.fast_fill.key(),

In addition there are several places in the codebase using msg! for logging. These case seem mostly to be outside of the general auction flow, and so might not have a security impact.

msg!() is mostly used in the context of executing admin actions. In this case a malicious admin could try to hide these artifacts. That said there is also a non-logging version of this function so they could just use this instead. In any case, there are other means for detecting an upgrade has changed.

So I think that this fix looks good so long as the last emit!() is addressed.

How does this look? 234d238

@mdulin2
Copy link

mdulin2 commented Jul 10, 2024

Is there any worry about too many nested calls? I think Solana has a maximum call depth of 4, where the emit_cpi does count for one of these.

@mdulin2
Copy link

mdulin2 commented Jul 10, 2024

With using Swap Layer, I want to make sure that the following isn't possible:

Payload Swap layer caller ->Swap Layer -> Token Router -> Matching Engine-> Emit CPI via nested contract call (goes over limit)

@johnsaigle
Copy link

johnsaigle commented Jul 11, 2024

@mdulin2 In that case, I think we'd expect the integration tests to fail on the Swap Layer side unless something is mocked in such a way that the calls don't really happen. Any thoughts on this @a5-pickle ?

@a5-pickle
Copy link
Contributor Author

a5-pickle commented Jul 11, 2024

@mdulin2 good call out regarding the call depth.

When @johnsaigle asked for the complete fast fill event to be converted to event CPI, I was thinking about the implication of this.

TL;DR nothing is affected by adding this event CPI.

We imagined that a program integrating with the Token Router would be composing with the consume prepared fill instruction (not the redeem fast fill instruction). Redeem fast fill creates a prepared fill, which needs to be consumed using the consume prepared fill instruction (and we did it this way to address the call depth issue). Check out swap layer's complete transfer/swap instructions as an example of how this works (and swap layer has its own redeeming logic spread out across complete transfer/swap and release inbound instructions).

But to address the call stack with redeem fast fill:

Redeem fast fill (Token Router) -> complete fast fill (Matching Engine) -> event CPI.

It was already at this call depth due to SPL Token program's transfer call. So having this event CPI should not add to the call depth.

@mdulin2
Copy link

mdulin2 commented Jul 11, 2024

Both logs and CPI events are being used. From internal chats, this is intentional because real time notifications are not possible through CPI events. So, the thought is that the regular logs can be the initial attempt but there would be a fallback mechanism similar to how the Wormhole Guardian works to see these anyway.

There's not really a good mechanism to fix this besides asking integrators to do multiple things to prevent weird auction related attacks.

Copy link

@mdulin2 mdulin2 left a comment

Choose a reason for hiding this comment

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

Solana event management is bad. This seems like the best way to handle stuff. As long as we convey the issues to the integrators, this looks good to me.

@a5-pickle a5-pickle merged commit b1593e0 into main Oct 7, 2024
6 checks passed
@a5-pickle a5-pickle deleted the event-cpi branch October 7, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants