Add safety comments (#8)

* Remove spl-token test cases

* Update CU values

* Add safety comments

* Use git dependencies

Cargo.toml

@@ -15,6 +15,6 @@ license = "Apache-2.0"
 repository = "https://github.com/febo/token"
 
 [workspace.dependencies]
-pinocchio = "0.7.0"
-pinocchio-log = "0.3.0"
-pinocchio-pubkey = "0.2.2"
+pinocchio = { version = "0.7", git = "https://github.com/febo/pinocchio.git", branch = "febo/close-unstable" }
+pinocchio-log = { version = "0.3", git = "https://github.com/febo/pinocchio.git", branch = "febo/close-unstable" }
+pinocchio-pubkey = { version = "0.2", git = "https://github.com/febo/pinocchio.git", branch = "febo/close-unstable" }

README.md

@@ -34,30 +34,30 @@ This repository contains a **proof-of-concept** of a reimplementation of the SPL
 | Instruction                | Completed | CU (`p-token`) | CU (`spl-token`) |
 |----------------------------|-----------|----------------|------------------|
 | `InitializeMint`           | ✅        | 100            | 2967             |
-| `InitializeAccount`        | ✅        | 170            | 4527             |
-| `InitializeMultisig`       | ✅        | 190            | 2973             |
-| `Transfer`                 | ✅        | 153            | 4645             |
-| `Approve`                  | ✅        | 124            | 2904             |
+| `InitializeAccount`        | ✅        | 185            | 4527             |
+| `InitializeMultisig`       | ✅        | 204            | 2973             |
+| `Transfer`                 | ✅        | 155            | 4645             |
+| `Approve`                  | ✅        | 122            | 2904             |
 | `Revoke`                   | ✅        |  97            | 2677             |
-| `SetAuthority`             | ✅        | 126            | 3167             |
-| `MintTo`                   | ✅        | 154            | 4538             |
+| `SetAuthority`             | ✅        | 127            | 3167             |
+| `MintTo`                   | ✅        | 155            | 4538             |
 | `Burn`                     | ✅        | 168            | 4753             |
-| `CloseAccount`             | ✅        | 147            | 2916             |
+| `CloseAccount`             | ✅        | 154            | 2916             |
 | `FreezeAccount`            | ✅        | 136            | 4265             |
 | `ThawAccount`              | ✅        | 136            | 4267             |
-| `TransferChecked`          | ✅        | 206            | 6201             |
+| `TransferChecked`          | ✅        | 204            | 6201             |
 | `ApproveChecked`           | ✅        | 162            | 4459             |
 | `MintToChecked`            | ✅        | 164            | 4546             |
-| `BurnChecked`              | ✅        | 170            | 4755             |
-| `InitializeAccount2`       | ✅        | 150            | 4388             |
+| `BurnChecked`              | ✅        | 169            | 4755             |
+| `InitializeAccount2`       | ✅        | 164            | 4388             |
 | `SyncNative`               | ✅        |                |                  |
 | `InitializeAccount3`       | ✅        | 272            | 4240             |
 | `InitializeMultisig2`      | ✅        | 319            | 2826             |
 | `InitializeMint2`          | ✅        | 234            | 2827             |
 | `GetAccountDataSize`       | ✅        |                |                  |
 | `InitializeImmutableOwner` | ✅        |                |                  |
-| `AmountToUiAmount`         | ✅        | 483            | 2501             |
-| `UiAmountToAmount`         | ✅        | 873            | 3161             |
+| `AmountToUiAmount`         | ✅        | 503            | 2501             |
+| `UiAmountToAmount`         | ✅        | 875            | 3161             |
 
 > Tests were run using Solana `v2.1.0`.
 

program/Cargo.toml

@@ -20,7 +20,6 @@ test-sbf = []
 [dependencies]
 pinocchio = { workspace = true }
 pinocchio-log = { workspace = true }
-pinocchio-pubkey = { workspace = true }
 token-interface = { version = "^0", path = "../interface" }
 
 [dev-dependencies]

program/src/processor/amount_to_ui_amount.rs

@@ -23,7 +23,8 @@ pub fn process_amount_to_ui_amount(
 
     let mint_info = accounts.first().ok_or(ProgramError::NotEnoughAccountKeys)?;
     check_account_owner(mint_info)?;
-    // SAFETY: there is a single borrow to the `Mint` account.
+    // SAFETY: single immutable borrow to `mint_info` account data and
+    // `load` validates that the mint is initialized.
     let mint = unsafe {
         load::<Mint>(mint_info.borrow_data_unchecked()).map_err(|_| TokenError::InvalidMint)?
     };

program/src/processor/burn_checked.rs

@@ -4,20 +4,20 @@ use super::shared;
 
 #[inline(always)]
 pub fn process_burn_checked(accounts: &[AccountInfo], instruction_data: &[u8]) -> ProgramResult {
-    let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
-    let amount = u64::from_le_bytes(
-        amount
-            .try_into()
-            .map_err(|_error| ProgramError::InvalidInstructionData)?,
-    );
+    // expected u64 (8) + u8 (1)
+    let (amount, decimals) = if instruction_data.len() == 9 {
+        let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
+        (
+            u64::from_le_bytes(
+                amount
+                    .try_into()
+                    .map_err(|_error| ProgramError::InvalidInstructionData)?,
+            ),
+            decimals.first(),
+        )
+    } else {
+        return Err(ProgramError::InvalidInstructionData);
+    };
 
-    shared::burn::process_burn(
-        accounts,
-        amount,
-        Some(
-            *decimals
-                .first()
-                .ok_or(ProgramError::InvalidInstructionData)?,
-        ),
-    )
+    shared::burn::process_burn(accounts, amount, decimals.copied())
 }

program/src/processor/close_account.rs

@@ -3,14 +3,16 @@ use pinocchio::{
 };
 use token_interface::{
     error::TokenError,
-    state::{account::Account, load_mut},
+    state::{account::Account, load},
 };
 
 use super::validate_owner;
 
-/// Incinerator address.
-const INCINERATOR_ID: Pubkey =
-    pinocchio_pubkey::pubkey!("1nc1nerator11111111111111111111111111111111");
+/// Incinerator (`1nc1nerator11111111111111111111111111111111`) address.
+const INCINERATOR_ID: Pubkey = [
+    0, 51, 144, 114, 141, 52, 17, 96, 121, 189, 201, 17, 191, 255, 0, 219, 212, 77, 46, 205, 204,
+    247, 156, 166, 225, 0, 56, 225, 0, 0, 0, 0,
+];
 
 #[inline(always)]
 pub fn process_close_account(accounts: &[AccountInfo]) -> ProgramResult {
@@ -24,26 +26,30 @@ pub fn process_close_account(accounts: &[AccountInfo]) -> ProgramResult {
     // raw pointer.
     if source_account_info == destination_account_info {
         return Err(ProgramError::InvalidAccountData);
-    }
-
-    let source_account =
-        unsafe { load_mut::<Account>(source_account_info.borrow_mut_data_unchecked())? };
-
-    if !source_account.is_native() && source_account.amount() != 0 {
-        return Err(TokenError::NonNativeHasBalance.into());
-    }
-
-    let authority = source_account
-        .close_authority()
-        .unwrap_or(&source_account.owner);
-
-    if !source_account.is_owned_by_system_program_or_incinerator() {
-        validate_owner(authority, authority_info, remaining)?;
-    } else if destination_account_info.key() != &INCINERATOR_ID {
-        return Err(ProgramError::InvalidAccountData);
+    } else {
+        // SAFETY: scoped immutable borrow to `source_account_info` account data and
+        // `load` validates that the account is initialized.
+        let source_account =
+            unsafe { load::<Account>(source_account_info.borrow_data_unchecked())? };
+
+        if !source_account.is_native() && source_account.amount() != 0 {
+            return Err(TokenError::NonNativeHasBalance.into());
+        }
+
+        let authority = source_account
+            .close_authority()
+            .unwrap_or(&source_account.owner);
+
+        if !source_account.is_owned_by_system_program_or_incinerator() {
+            validate_owner(authority, authority_info, remaining)?;
+        } else if destination_account_info.key() != &INCINERATOR_ID {
+            return Err(ProgramError::InvalidAccountData);
+        }
     }
 
     let destination_starting_lamports = destination_account_info.lamports();
+    // SAFETY: single mutable borrow to `destination_account_info` lamports and
+    // there are no "active" borrows of `source_account_info` account data.
     unsafe {
         // Moves the lamports to the destination account.
         *destination_account_info.borrow_mut_lamports_unchecked() = destination_starting_lamports

program/src/processor/get_account_data_size.rs

@@ -17,6 +17,8 @@ pub fn process_get_account_data_size(accounts: &[AccountInfo]) -> ProgramResult
     // Make sure the mint is valid.
     check_account_owner(mint_info)?;
 
+    // SAFETY: single immutable borrow to `mint_info` account data and
+    // `load` validates that the mint is initialized.
     let _ = unsafe {
         load::<Mint>(mint_info.borrow_data_unchecked()).map_err(|_| TokenError::InvalidMint)?
     };

program/src/processor/initialize_account2.rs

@@ -1,4 +1,9 @@
-use pinocchio::{account_info::AccountInfo, pubkey::Pubkey, ProgramResult};
+use pinocchio::{
+    account_info::AccountInfo,
+    program_error::ProgramError,
+    pubkey::{Pubkey, PUBKEY_BYTES},
+    ProgramResult,
+};
 
 use super::shared;
 
@@ -7,6 +12,13 @@ pub fn process_initialize_account2(
     accounts: &[AccountInfo],
     instruction_data: &[u8],
 ) -> ProgramResult {
-    let owner = unsafe { &*(instruction_data.as_ptr() as *const Pubkey) };
+    // SAFETY: validate `instruction_data` length.
+    let owner = unsafe {
+        if instruction_data.len() != PUBKEY_BYTES {
+            return Err(ProgramError::InvalidInstructionData);
+        } else {
+            &*(instruction_data.as_ptr() as *const Pubkey)
+        }
+    };
     shared::initialize_account::process_initialize_account(accounts, Some(owner), true)
 }

program/src/processor/initialize_account3.rs

@@ -1,4 +1,9 @@
-use pinocchio::{account_info::AccountInfo, pubkey::Pubkey, ProgramResult};
+use pinocchio::{
+    account_info::AccountInfo,
+    program_error::ProgramError,
+    pubkey::{Pubkey, PUBKEY_BYTES},
+    ProgramResult,
+};
 
 use super::shared;
 
@@ -7,6 +12,13 @@ pub fn process_initialize_account3(
     accounts: &[AccountInfo],
     instruction_data: &[u8],
 ) -> ProgramResult {
-    let owner = unsafe { &*(instruction_data.as_ptr() as *const Pubkey) };
+    // SAFETY: validate `instruction_data` length.
+    let owner = unsafe {
+        if instruction_data.len() != PUBKEY_BYTES {
+            return Err(ProgramError::InvalidInstructionData);
+        } else {
+            &*(instruction_data.as_ptr() as *const Pubkey)
+        }
+    };
     shared::initialize_account::process_initialize_account(accounts, Some(owner), false)
 }

program/src/processor/initialize_immutable_owner.rs

@@ -8,6 +8,7 @@ use token_interface::{
 pub fn process_initialize_immutable_owner(accounts: &[AccountInfo]) -> ProgramResult {
     let token_account_info = accounts.first().ok_or(ProgramError::NotEnoughAccountKeys)?;
 
+    // SAFETY: single immutable borrow to `token_account_info` account data.
     let account = unsafe { load_unchecked::<Account>(token_account_info.borrow_data_unchecked())? };
 
     if account.is_initialized() {

program/src/processor/initialize_mint.rs

@@ -35,6 +35,7 @@ pub fn process_initialize_mint(
         (mint_info, None)
     };
 
+    // SAFETY: single mutable borrow to `mint_info` account data.
     let mint = unsafe { load_mut_unchecked::<Mint>(mint_info.borrow_mut_data_unchecked())? };
 
     if mint.is_initialized() {
@@ -44,7 +45,9 @@ pub fn process_initialize_mint(
     // Check rent-exempt status of the mint account.
 
     let is_exempt = if let Some(rent_sysvar_info) = rent_sysvar_info {
-        let rent = unsafe { Rent::from_bytes(rent_sysvar_info.borrow_data_unchecked()) };
+        // SAFETY: single immutable borrow to `rent_sysvar_info`; account ID and length are
+        // checked by `from_account_info_unchecked`.
+        let rent = unsafe { Rent::from_account_info_unchecked(rent_sysvar_info)? };
         rent.is_exempt(mint_info.lamports(), size_of::<Mint>())
     } else {
         Rent::get()?.is_exempt(mint_info.lamports(), size_of::<Mint>())
@@ -81,7 +84,7 @@ impl InitializeMint<'_> {
         // - decimals (1 byte)
         // - mint_authority (32 bytes)
         // - option + freeze_authority (1 byte + 32 bytes)
-        if bytes.len() < 34 {
+        if bytes.len() < 34 || (bytes[33] == 1 && bytes.len() < 66) {
             return Err(ProgramError::InvalidInstructionData);
         }
 
@@ -93,16 +96,19 @@ impl InitializeMint<'_> {
 
     #[inline]
     pub fn decimals(&self) -> u8 {
+        // SAFETY: the `bytes` length was validated in `try_from_bytes`.
         unsafe { *self.raw }
     }
 
     #[inline]
     pub fn mint_authority(&self) -> &Pubkey {
+        // SAFETY: the `bytes` length was validated in `try_from_bytes`.
         unsafe { &*(self.raw.add(1) as *const Pubkey) }
     }
 
     #[inline]
     pub fn freeze_authority(&self) -> Option<&Pubkey> {
+        // SAFETY: the `bytes` length was validated in `try_from_bytes`.
         unsafe {
             if *self.raw.add(33) == 0 {
                 Option::None

program/src/processor/mint_to_checked.rs

@@ -4,17 +4,20 @@ use super::shared;
 
 #[inline(always)]
 pub fn process_mint_to_checked(accounts: &[AccountInfo], instruction_data: &[u8]) -> ProgramResult {
-    let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
+    // expected u64 (8) + u8 (1)
+    let (amount, decimals) = if instruction_data.len() == 9 {
+        let (amount, decimals) = instruction_data.split_at(core::mem::size_of::<u64>());
+        (
+            u64::from_le_bytes(
+                amount
+                    .try_into()
+                    .map_err(|_error| ProgramError::InvalidInstructionData)?,
+            ),
+            decimals.first(),
+        )
+    } else {
+        return Err(ProgramError::InvalidInstructionData);
+    };
 
-    let amount = u64::from_le_bytes(
-        amount
-            .try_into()
-            .map_err(|_error| ProgramError::InvalidInstructionData)?,
-    );
-
-    shared::mint_to::process_mint_to(
-        accounts,
-        amount,
-        Some(*decimals.first().ok_or(ProgramError::InvalidAccountData)?),
-    )
+    shared::mint_to::process_mint_to(accounts, amount, decimals.copied())
 }

program/src/processor/mod.rs

@@ -92,7 +92,10 @@ fn check_account_owner(account_info: &AccountInfo) -> ProgramResult {
     }
 }
 
-/// Validates owner(s) are present
+/// Validates owner(s) are present.
+///
+/// Note that `owner_account_info` will be immutable borrowed when it represents
+/// a multisig account.
 #[inline(always)]
 fn validate_owner(
     expected_owner: &Pubkey,
@@ -106,6 +109,8 @@ fn validate_owner(
     if owner_account_info.data_len() == Multisig::LEN
         && owner_account_info.owner() == &TOKEN_PROGRAM_ID
     {
+        // SAFETY: the caller guarantees that there are no mutable borrows of `owner_account_info`
+        // account data and the `load` validates that the account is initialized.
         let multisig = unsafe { load::<Multisig>(owner_account_info.borrow_data_unchecked())? };
 
         let mut num_signers = 0;