Merge pull request #181 from crytic/nssc-solana-sysvar

NSSC Solana

Author: montyly

README.md

@@ -18,6 +18,7 @@ Follow our guidelines and best practices to write secure smart contracts.
    - [Cairo](./not-so-smart-contracts/cairo)
    - [Cosmos](./not-so-smart-contracts/cosmos)
    - [Substrate](./not-so-smart-contracts/substrate)
+   - [Solana](./not-so-smart-contracts/solana)
 - [Program analysis](./program-analysis): How to use automated tools to secure contracts
   - [Echidna](./program-analysis/echidna): a fuzzer that will check your contract's properties.
   - [Slither](./program-analysis/slither): a static analyzer avaialable through a CLI and scriptable interface.

not-so-smart-contracts/solana/README.md

@@ -0,0 +1,28 @@
+# (Not So) Smart Contracts
+
+This repository contains examples of common Solana smart contract vulnerabilities, including code from real smart contracts. Use Not So Smart Contracts to learn about Solana vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools.
+
+## Features
+
+Each _Not So Smart Contract_ includes a standard set of information:
+
+* Description of the vulnerability type
+* Attack scenarios to exploit the vulnerability
+* Recommendations to eliminate or mitigate the vulnerability
+* Real-world contracts that exhibit the flaw
+* References to third-party resources with more information
+
+## Vulnerabilities
+| Not So Smart Contract | Description |
+| --- | --- |
+| [Arbitrary CPI](arbitrary_cpi) | Arbitrary program account passed in upon invocation |
+| [Improper PDA Validation](improper_pda_validation) | PDAs are vulnerable to being spoofed via bump seeds |
+| [Ownership Check](ownership_check) | Broken access control due to missing ownership validation |
+| [Signer Check](signer_check) | Broken access control due to missing signer validation |
+| [Sysvar Account Check](sysvar_account_check) | Sysvar accounts are vulnerable to being spoofed |
+
+## Credits
+
+These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/).
+
+If you have questions, problems, or just want to learn more, then join the #solana channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly.

not-so-smart-contracts/solana/arbitrary_cpi/README.md

@@ -0,0 +1,39 @@
+# Arbitrary CPI
+Solana allows programs to call one another through cross-program invocation (CPI). This can be done via `invoke`, which is responsible for routing the passed in instruction to the program. Whenever an external contract is invoked via CPI, the program must check and verify the program ID. If the program ID isn't verified, then the contract can be called into an attacker-controlled contract instead of the intended one.
+
+View ToB's lint implementation for the arbitrary CPI issue [here](https://github.com/crytic/solana-lints/tree/master/lints/arbitrary_cpi).
+
+## Exploit Scenario
+Consider the following `withdraw` function. Tokens are able to be withdrawn from the pool to a user account. The program invoked here is user-controlled and there's no check that program passed in is the intended `token_program`. This allows a malicious user to pass in their own program with functionality to their discretion - such as draining the pool of the inputted `amount` tokens. 
+### Example Contract
+```rust
+   pub fn withdraw(accounts: &[AccountInfo], amount: u64) -> ProgramResult {
+        let account_info_iter = &mut accounts.iter();
+        let token_program = next_account_info(account_info_iter)?;
+        let pool = next_account_info(account_info_iter)?;
+        let pool_auth = next_account_info(account_info_iter)?;
+        let destination = next_account_info(account_info_iter)?;
+        invoke(
+            &spl_token::instruction::transfer(
+                &token_program.key,
+                &pool.key,
+                &destination.key,
+                &pool_auth.key,
+                &[],
+                amount,
+            )?,
+            &[
+                &pool.clone(),
+                &destination.clone(),
+                &pool_auth.clone(),
+            ],
+        )
+    }
+```
+*Inspired by [Sealevel](https://github.com/coral-xyz/sealevel-attacks/)*
+## Mitigation
+```rust
+        if INPUTTED_PROGRAM.key != &INTENDED_PROGRAM::id() {
+            return Err(ProgramError::InvalidProgramId);
+        }   
+```

not-so-smart-contracts/solana/improper_pda_validation/README.md

@@ -0,0 +1,34 @@
+
+# Improper PDA bump seed validation
+
+PDAs (Program Derived Addresses) are, by definition, [program-controlled](https://docs.solana.com/terminology#program-derived-account-pda) accounts and therefore can be used to sign without the need to provide a private key. PDAs are generated through a set of seeds and a program id, which are then collectively hashed to verify that the key doesn't lie on the ed25519 curve (curve used by Solana to sign transactions). 
+
+Values on this elliptic curve have a corresponding private key, which wouldn't make it a PDA. In the case a public key lying on the elliptic curve is found, our 32-byte address is modified through the addition of a bump to "bump" it off the curve. A bump, represented by a singular byte iterating through 255 to 0 is added onto our input until an address that doesn’t lie on the elliptic curve is generated, meaning that we’ve found an address without an associated private key. 
+
+The issue arises with seeds being able to have multiple bumps, thus allowing varying PDAs that are valid from the same seed. An attacker can create a PDA with fabricated data - the program ID and seeds are the same as for the expected PDA but with different bump seeds. Without any explicit check against the bump seed itself, the program leaves itself vulnerable to the attacker tricking the program into thinking they’re the expected PDA and thus interacting with the contract on behalf of them.
+
+View ToB's lint implementation for the bump seed canonicalization issue [here](https://github.com/crytic/solana-lints/tree/master/lints/bump_seed_canonicalization).
+
+## Exploit Scenario
+
+In Solana, the `create_program_address` function creates a 32-byte address based off the set of seeds and program address. On it's own, the address may lie on the ed25519 curve. Consider the following without any other validation being referenced within a sensitive function, such as one that handles transfers. That PDA could be spoofed by a passed in user-controlled PDA with malicious functions to their own discretion.  
+### Example Contract
+```rust 
+let program_address = Pubkey::create_program_address(&[key.to_le_bytes().as_ref(), &[reserve_bump]], program_id)?;
+
+...
+```
+## Mitigation
+
+The `find_program_address` function finds the largest bump seeds for which there exists a corresponding PDA (i.e., an address not on the ed25519 curve), and returns both the address and the bump seed. The function panics in the case that no PDA address can be found.
+
+```rust
+        let (address, system_bump) = Pubkey::find_program_address(&[key.to_le_bytes().as_ref()], program_id);
+
+        if program_address != &account_data.key() {
+            return Err(ProgramError::InvalidAddress);
+        }
+        if system_bump != reserve_bump {
+            return Err(ProgramError::InvalidBump);
+        }
+```

not-so-smart-contracts/solana/ownership_check/README.md

@@ -0,0 +1,39 @@
+# Missing Ownership Check
+Accounts in Solana include metadata of an owner. These owners are identified by their own program ID. Without sufficient checks that the expected program ID matches that of the passed in account, an attacker can fabricate account data to pass any other preconditions and bypass the ownership check. 
+
+This malicious account will inherently have a different program ID, but considering there’s no check that the program ID is the same, as long as the other preconditions are passed the attacker can trick the program into thinking their malicious account is the expected authorized account.
+
+## Exploit Scenario
+The following contract updates the current market owner with a new one. Unfortunately, the only check being done here is against the current owner’s public key prior to setting a new owner. 
+Therefore, a malicious actor can set themselves as the new owner without being the actual market owner. This is because the ownership of the market owner account isn’t being fully verified against itself by program ID. Since there’s no check that the market is the one owned by the program itself, an attacker can pass in their own fabricated account data which is then verified against a public key of the current owner’s account, making it easy to set themselves as the new owner. 
+
+### Example Contract 
+```rust
+fn set_owner(program_id: &Pubkey, new_owner: Pubkey, accounts: &[AccountInfo]) -> ProgramResult {
+	let account_info_iter = &mut accounts.iter();
+	let market_info = next_account_info(account_info_iter)?;
+	let current_owner = next_account_info(account_info_iter)?;
+
+	let mut market = Market::unpack(&market_info.data.borrow())?;
+ 
+	if &market.owner != current_owner.pubkey {
+    	    return Err(InvalidMarketOwner.into());
+	}
+	market.owner = new_owner;
+
+  ...
+ 
+	Ok(())
+
+}
+```
+*Inspired by [SPL Lending Program](https://github.com/solana-labs/solana-program-library/tree/master/token-lending/program)*
+
+## Mitigation  
+
+```rust
+  	if EXPECTED_ACCOUNT.owner != program_id {
+    	    return Err(ProgramError::IncorrectProgramId);
+	}
+```
+For further reading on different forms of account verification in Solana and implementation refer to the [Solana Cookbook](https://solanacookbook.com/references/programs.html#how-to-verify-accounts).

not-so-smart-contracts/solana/signer_check/README.md

@@ -0,0 +1,39 @@
+# Missing Signer Check
+In Solana, a [transaction](https://docs.solana.com/developing/programming-model/transactions)  lists all account public keys used by the instructions of the transaction. These public keys come with a signature, ensuring that the account holder has authorized the transaction.
+
+Specifically, these signatures used to validate transactions are generated by the account’s private key and are verified by being matched against an inputted public key prior to successful transaction execution.
+In case certain permissions are required to perform a sensitive function of the contract, a missing signer check becomes an issue. Any user’s account with the authorized account’s public key will be able to call the respective access controlled functions permissionlessly without ever having to validate their private key.
+
+## Exploit Scenario
+The following contract updates the current market owner with a new one. Unfortunately, the only check being done here is against the current owner’s public key prior to setting a new owner. 
+Therefore, a malicious actor can set themselves as the new owner without being the actual market owner. This is because the current owner’s private key isn’t being verified considering the contract doesn’t check whether the account holder has signed or not.
+
+### Example Contract 
+```rust
+fn set_owner(program_id: &Pubkey, new_owner: Pubkey, accounts: &[AccountInfo]) -> ProgramResult {
+	let account_info_iter = &mut accounts.iter();
+	let market_info = next_account_info(account_info_iter)?;
+	let current_owner = next_account_info(account_info_iter)?;
+
+	let mut market = Market::unpack(&market_info.data.borrow())?;
+ 
+	if &market.owner != current_owner.pubkey {
+    	    return Err(InvalidMarketOwner.into());
+	}
+	market.owner = new_owner;
+
+  ...
+ 
+	Ok(())
+
+}
+```
+*Inspired by [SPL Lending Program](https://github.com/solana-labs/solana-program-library/tree/master/token-lending/program)*
+
+## Mitigation
+```rust
+  	if !EXPECTED_ACCOUNT.is_signer {
+    	return Err(ProgramError::MissingRequiredSignature);
+	}
+```
+For further reading on different forms of account verification in Solana and implementation refer to the [Solana Cookbook](https://solanacookbook.com/references/programs.html#how-to-verify-accounts).

not-so-smart-contracts/solana/sysvar_account_check/README.md

@@ -0,0 +1,71 @@
+# Missing Sysvar Account Check
+The sysvar (system account) account is often used while validating access control for restricted functions by confirming that the inputted sysvar account by the user matches up with the expected sysvar account. Without this check in place, any user is capable of passing in their own spoofed sysvar account and in turn bypassing any further authentication associated with it, causing potentially disastrous effects.
+
+## Exploit Scenario
+
+secp256k1 is an elliptic curve used by a number of blockchains for signatures. Validating signatures is crucial as by bypassing signature checks, an attacker can gain access to restricted functions that could lead to drainage of funds. 
+
+Here, `load_current_index` and `load_instruction_at` are functions that don't verify that the inputted sysvar account is authorized, therefore allowing serialized maliciously fabricated data to sucessfully spoof as an authorized secp256k1 signature. 
+
+### Example Contract 
+```rust
+pub fn verify_signatures(account_info: &AccountInfo) -> ProgramResult {
+    let index = solana_program::sysvar::instructions::load_current_index(
+        &account_info.try_borrow_mut_data()?,
+    );
+
+    let secp_instruction = sysvar::instructions::load_instruction_at(
+        (index - 1) as usize,
+        &account_info.try_borrow_mut_data()?,
+    );
+    if secp_instruction.program_id != secp256k1_program::id() {
+        return Err(InvalidSecpInstruction.into());
+    }
+    ...
+}
+```
+Refer to [Mitigation](https://github.com/crytic/building-secure-contracts/tree/master/not-so-smart-contracts/solana/sysvar_account_check#Mitigation) to understand what's wrong with these functions and how sysvar account checks were added.
+
+## Mitigation
+- Solana libraries should be running on version 1.8.1 and up
+- Use `load_instruction_at_checked` and `load_current_index_checked`
+
+Utilizing the latest Solana version and referencing checked functions, especially on sensitive parts of a contract is crucial even if potential attack vectors have been fixed post-audit. 
+Leaving the system exposed to any point of failure compromises the entire system's integrity, especially while the contracts are being constantly updated.
+
+Here is the code showing the sysvar account checks added between unchecked and checked functions:
+- [load_instruction_at](https://docs.rs/solana-program/1.13.5/src/solana_program/sysvar/instructions.rs.html#186-188) vs [load_instruction_at_checked](https://docs.rs/solana-program/1.13.5/src/solana_program/sysvar/instructions.rs.html#192-205)
+- [load_current_index](https://docs.rs/solana-program/1.13.5/src/solana_program/sysvar/instructions.rs.html#107-112) vs [load_current_index_checked](https://docs.rs/solana-program/1.13.5/src/solana_program/sysvar/instructions.rs.html#116-128)
+
+---
+## Example: Wormhole Exploit (February 2022)
+### Funds lost: ~326,000,000 USD
+
+**Note: The following analysis is condensed down to be present this attack vector as clearly as possible, and certain details might’ve been left out for the sake of simplification**
+
+The Wormhole hack serves to be one of the most memorable exploits in terms of impact DeFi has ever seen. 
+
+This exploit also happens to incorporate a missing sysvar account check that allowed the attacker to:
+1. Spoof Guardian signatures as valid
+2. Use them to create a Validator Action Approval (VAA)
+3. Mint 120,000 ETH via calling complete_wrapped function
+
+(These actions are all chronologically dependent on one another based on permissions and conditions - this analysis will only dive into “Step 1”)
+
+The SignatureSet was able to be faked because the `verify_signatures` function failed to appropriately [verify](https://github.com/wormhole-foundation/wormhole/blob/ca509f2d73c0780e8516ffdfcaf90b38ab6db203/solana/bridge/program/src/api/verify_signature.rs#L101
+) the sysvar account passed in:
+
+```rust
+let secp_ix = solana_program::sysvar::instructions::load_instruction_at(
+    secp_ix_index as usize,
+    &accs.instruction_acc.try_borrow_mut_data()?,
+)
+```
+`load_instruction_at` doesn't verify that the inputted data came from the authorized sysvar account.
+
+The fix for this was to upgrade the Solana version and get rid of these unsafe deprecated functions (see [Mitigation](https://github.com/crytic/building-secure-contracts/tree/master/not-so-smart-contracts/solana/sysvar_account_check#Mitigation)). Wormhole had [caught](https://github.com/wormhole-foundation/wormhole/commit/7edbbd3677ee6ca681be8722a607bc576a3912c8#diff-0d27d8889edd071b86d3f3299276882d97613ad6ab3b0b6412ae4ebf3ccd6370R92-R103)  this issue but didn't update their deployed contracts in time before the exploiter had already managed to drain funds.
+
+## Resources:
+[samczsun's Wormhole exploit breakdown thread](https://twitter.com/samczsun/status/1489044939732406275)
+
+