coral-xyz/sealevel-attacks

☠️ Common Security Exploits and Protections on Solana

sealevel-attacks's Issues

Need help about insecure, insecure-still and insecure-still-still cases. What makes the code unsafe?

Error in secure version of 7-bump-seed-canonicalization

I think that the following snippet from 7-bump-seed-canonicalization

        let (address, expected_bump) =
            Pubkey::find_program_address(&[key.to_le_bytes().as_ref(), &[bump]], ctx.program_id);

implies that expected_bump != bump.

The reason is that while the Pubkey::create_program_address expects the bump as the last item in the seeds argument, the find_program_address would just use it as another seed and result in a different bump. (See also find_program_address, and create_program_address).

So I think this should be changed to

        let (address, expected_bump) =
            Pubkey::find_program_address(&[key.to_le_bytes().as_ref()], ctx.program_id);

Is secure better than recommended?

Each example contains 3 folders: insecure, recommended, and secure. It might be the alphabetical ordering, but it isn't quite clear which folder is "best".

One interpretation is that recommended means the minimum recommendation and that secure is better. The other is that secure is the minimum required to stop the attack but recommended is better.

Is recommended > secure or is secure > recommended.

edit: lol I'm not looking for an answer, I'm looking for a discussion whether it's just me having these thoughts and on how to make it more clear (if needed). My initial thoughts are adding a number to the folder name, or replacing secure with minimum, or a simple note on the README.md.

Example #2 Owner Checks

In this example, the account was deserialized with the SplTokenAccount util and owner check is performed after. Why is this example insecure other than not deserializing with the Anchor TokenAccount struct?

Misleading secure example for 4-initialization

I believe there are couple issues with the secure example for 4-initialization attack.

1. The discriminator field in the User struct is a misleading field name. It is not used in the code as a discriminator, in the sense that type-cosplay calls for, ie, to uniquely differentiate accounts. Further, a bool cannot even be used as a proper discriminant. It seems like the field should be renamed to is_initialized, because that is what it seems like it is being used for, an initialization flag.
2. If the purpose of the discriminator field is indeed intended to be used as an "initialization flag", there should not be a boolean NOT operator on line 13. If user.discriminator is false, ie, uninitialized, then it should be initialized. However, the logic dictates that if it is false, then the code returns an error. If it is true (initialized), then it is reinitialized. This is the opposite of what we want I believe.

Recommended vs. secure

Sorry if I am being dense, but what are the differences between the subdirectories "recommended" and "secure"? (Presumably, "secure" code would also be "recommended"?)

Arithmetic overflow/underflow protection examples

We should have some examples showing how to implement mathematical operations using checked_add, checked_sub, saturating_pow etc

Pda sharing 8 secure and insecure have the same code

I believe the secure is missing the seeds and bumbs account checks on WithdrawTokens

DataWithBump never used in example

https://github.com/project-serum/sealevel-attacks/blob/609e5ade229eaa2b030589020e840c9407bda027/programs/7-bump-seed-canonicalization/secure/src/lib.rs#L41

Missing documentation for examples

@armaniferrante we should add one/two liner comments or doc comments on every example

explaining
unsafe - why it's unsafe
recommended - why this practice is recommended
secure - why it's secure.

It'll definitely help new repo visitor understand the moto behind the different approaches and will help get deeper understanding overall.

Need to import ProgramResult type for 1-account-data-matching ui tests

In the ui tests, for example for recommended: https://github.com/project-serum/sealevel-attacks/blob/master/programs/1-account-data-matching/recommended/src/lib.rs, it refers to ProgramResult but doesn't import it.

Need to import it somehow like: use anchor_lang::solana_program::entrypoint::ProgramResult;. This goes for all ui tests for this particular attack.

Add sysvar address checking example

Need help understanding 9-closing-accounts secure vs insecure-still-still

Sorry I don't quite see what the purpose of the force_defund() instruction is or how it makes it secure in contrast to insecure-still-still. From its naming it seems like it's supposed to force the defunding of closed accounts? If so, should it be

if discriminator != CLOSED_ACCOUNT_DISCRIMINATOR

on https://github.com/project-serum/sealevel-attacks/blob/2902c7976a36ddd34e16022026e8d64cf1667974/programs/9-closing-accounts/secure/src/lib.rs#L41-L43 instead?

Is the fact that it's a different instruction supposed to demonstrate that the only way to force the defunding of a closed account is to start a new transaction that calls force_defund() after close() has been called in a previous transaction (because otherwise users are free to append TransferLamports instructions to the same transaction in which close() was called)?