Merge pull request #14 from Conflux-Chain/fixAudit2

Fix audit2

src/app_ui/menu.rs

@@ -32,8 +32,6 @@ use crate::Instruction;
 #[cfg(not(any(target_os = "stax", target_os = "flex")))]
 use ledger_device_sdk::io::Event;
 
-// use ledger_device_sdk::nvm::*;
-
 #[cfg(not(any(target_os = "stax", target_os = "flex")))]
 fn ui_about_menu(comm: &mut Comm) -> Event<Instruction> {
     let pages = [
@@ -54,7 +52,7 @@ fn ui_setting_menu(comm: &mut Comm) -> Event<Instruction> {
     let blind_signing_index = 0;
 
     let settings: Settings = Default::default();
-    let mut bs_enabled: bool = settings.get_element(blind_signing_index) != 0;
+    let mut bs_enabled: bool = settings.get_element(blind_signing_index).unwrap() != 0;
     let mut bs_status = if bs_enabled { "Enabled" } else { "Disabled" };
 
     loop {
@@ -68,11 +66,15 @@ fn ui_setting_menu(comm: &mut Comm) -> Event<Instruction> {
                 bs_enabled = !bs_enabled;
                 match bs_enabled {
                     true => {
-                        settings.set_element(blind_signing_index, 1);
+                        settings
+                            .set_element(blind_signing_index, 1)
+                            .expect("should success");
                         bs_status = "Enabled";
                     }
                     false => {
-                        settings.set_element(blind_signing_index, 0);
+                        settings
+                            .set_element(blind_signing_index, 0)
+                            .expect("should success");
                         bs_status = "Disabled";
                     }
                 }

src/app_ui/sign.rs

@@ -95,7 +95,7 @@ pub fn ui_display_tx(tx: &Transaction) -> Result<bool, AppSW> {
 
         // If first setting switch is disabled do not display the transaction data
         let settings: Settings = Default::default();
-        if settings.get_element(1) == 0 {
+        if settings.get_element(1)? == 0 {
             Ok(review.show(&my_fields[0..2]))
         } else {
             Ok(review.show(&my_fields))

src/cfx_addr/consts.rs

@@ -1,20 +1,3 @@
-pub const CHARSET_SIZE: usize = 32;
-
-pub const RESERVED_BITS_MASK: u8 = 0xf8;
-
-// Because we use a different CHARSET than BCH, it's OK that we disregard all of
-// the BITCOIN type bits.
-//
-// // pub const TYPE_MASK: u8 = 0x78;
-// // pub const TYPE_BITCOIN_P2PKH: u8 = 0x00;
-// // pub const TYPE_BITCOIN_P2SH: u8 = 0x08;
-//
-// In Conflux we have so far only one type of account key format. So we try to
-// use the 4 type bits differently. In the future we may use them in some
-// special transaction scenarios. e.g. A payment code, an address linked to
-// off-chain or cross-chain mechanism.
-
-pub const SIZE_MASK: u8 = 0x07;
 pub const SIZE_160: u8 = 0x00;
 
 // In Conflux we only have 160 bits hash size, however we keep these unused
@@ -27,8 +10,6 @@ pub const SIZE_384: u8 = 0x05;
 pub const SIZE_448: u8 = 0x06;
 pub const SIZE_512: u8 = 0x07;
 
-pub const BASE32_CHARS: &str = "abcdefghijklmnopqrstuvwxyz0123456789";
-pub const EXCLUDE_CHARS: [char; 4] = ['o', 'i', 'l', 'q'];
 pub const CHARSET: [char; 32] = [
     'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'j', 'k', 'm', 'n', 'p', 'r', 's', 't', 'u', 'v', 'w',
     'x', 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
@@ -37,14 +18,9 @@ pub const CHARSET: [char; 32] = [
 // network prefix
 pub const MAINNET_PREFIX: &str = "cfx";
 pub const TESTNET_PREFIX: &str = "cfxtest";
-pub const NETWORK_ID_PREFIX: &str = "net";
 
-// address types
-pub const ADDRESS_TYPE_BUILTIN: &str = "builtin";
-pub const ADDRESS_TYPE_CONTRACT: &str = "contract";
-pub const ADDRESS_TYPE_NULL: &str = "null";
-pub const ADDRESS_TYPE_UNKNOWN: &str = "unknown";
-pub const ADDRESS_TYPE_USER: &str = "user";
+pub const MAIN_NET_ID: u64 = 1029;
+pub const TEST_NET_ID: u64 = 1;
 
 // These two network_ids are reserved.
-pub const RESERVED_NETWORK_IDS: [u64; 2] = [1, 1029];
+pub const RESERVED_NETWORK_IDS: [u64; 2] = [TEST_NET_ID, MAIN_NET_ID];

src/cfx_addr/mod.rs

@@ -1,4 +1,3 @@
-#![allow(unused)]
 mod consts;
 mod types;
 mod utils;

src/cfx_addr/types.rs

@@ -1,4 +1,6 @@
-use super::consts::{MAINNET_PREFIX, RESERVED_NETWORK_IDS, TESTNET_PREFIX};
+use super::consts::{
+    MAINNET_PREFIX, MAIN_NET_ID, RESERVED_NETWORK_IDS, TESTNET_PREFIX, TEST_NET_ID,
+};
 use alloc::{format, string::String};
 use core::fmt;
 
@@ -29,8 +31,8 @@ impl Network {
 
     pub fn from_network_id(network_id: u64) -> Self {
         match network_id {
-            1029 => Self::Main,
-            1 => Self::Test,
+            MAIN_NET_ID => Self::Main,
+            TEST_NET_ID => Self::Test,
             _ => Self::Id(network_id),
         }
     }
@@ -39,17 +41,13 @@ impl Network {
 /// Error concerning encoding of cfx_base32_addr.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum EncodingError {
-    AddressType(u8),
     Length(usize),
     NetworkId(u64),
 }
 
 impl fmt::Display for EncodingError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Self::AddressType(type_byte) => {
-                write!(f, "unrecognized type bits 0x{:02x}", type_byte)
-            }
             Self::Length(length) => {
                 write!(f, "invalid length ({})", length)
             }
@@ -59,9 +57,3 @@ impl fmt::Display for EncodingError {
         }
     }
 }
-
-#[derive(Copy, Clone)]
-pub enum EncodingOptions {
-    Simple,
-    QrCode,
-}

src/cfx_addr/utils.rs

@@ -69,10 +69,9 @@ pub fn convert_bits(
             ret.push((acc << (outbits - num)) as u8);
         }
     } else {
-        // FIXME: add unit tests for it.
         // If there's some bits left, figure out if we need to remove padding
         // and add it
-        let padding = ((data.len() * inbits as usize) % outbits as usize) as u8;
+        let _padding = ((data.len() * inbits as usize) % outbits as usize) as u8;
         if num >= inbits || acc != 0 {
             return Err("InvalidPadding");
         }

src/consts.rs

@@ -1,46 +1,20 @@
-#![allow(dead_code)]
-
 pub const ADDRRESS_BYTES_LEN: usize = 20;
 
 pub const HASH_BYTES_LEN: usize = 32;
 
-/**
- * Maximum length of MAJOR_VERSION || MINOR_VERSION || PATCH_VERSION.
- */
-pub const APPVERSION_LEN: usize = 3;
-
-/**
- * Maximum length of application name.
- */
-pub const MAX_APPNAME_LEN: usize = 64;
-
 /**
  * Maximum transaction length (bytes).
  */
 pub const MAX_TRANSACTION_LEN: usize = 765;
 
-/**
- * Maximum signature length (bytes).
- */
-pub const MAX_DER_SIG_LEN: usize = 72;
-
 /**
  * Exponent used to convert Drip to CFX unit (N CFX = N * 10^18 Drip).
  */
 pub const EXPONENT_SMALLEST_UNIT: usize = 18;
 
-/**
- * Well-known Conflux chain IDs.
- */
-pub const CONFLUX_MAINNET_CHAINID: usize = 1029;
-
-pub const CONFLUX_TESTNET_CHAINID: usize = 1;
-
 /**
  * Application flags.
  */
 pub const APP_FLAG_BLIND_SIGNING_ENABLED: u8 = 0x01;
 
 pub const APP_FLAG_DETAILED_DISPLAY_ENABLED: u8 = 0x02;
-
-// pub const PERSONAL_SIGN_PREFIX: &str = "\x19Conflux Signed Message:\n";

src/handlers/get_public_key.rs

@@ -41,6 +41,7 @@ pub fn handler_get_public_key(
 
     let (k, cc) = Secp256k1::derive_from(path.as_ref());
     let pk = k.public_key().map_err(|_| AppSW::KeyDeriveFail)?;
+    drop(k);
 
     // Display address on device if requested
     if display {

src/handlers/get_version.rs

@@ -24,11 +24,11 @@ pub fn handler_get_version(comm: &mut io::Comm) -> Result<(), AppSW> {
     if let Some((major, minor, patch)) = parse_version_string(env!("CARGO_PKG_VERSION")) {
         let mut flags: u8 = 0b0000_0100;
         let settings: Settings = Default::default();
-        if settings.get_element(0) == 1 {
+        if settings.get_element(0)? == 1 {
             // If the first byte is 1, then the user has enabled the "Blind signing" setting
             flags |= crate::consts::APP_FLAG_BLIND_SIGNING_ENABLED;
         }
-        if settings.get_element(1) == 1 {
+        if settings.get_element(1)? == 1 {
             // If the first byte is 1, then the user has enabled the "Display data" setting
             flags |= crate::consts::APP_FLAG_DETAILED_DISPLAY_ENABLED;
         }

src/main.rs

@@ -69,7 +69,7 @@ const P1_SIGN_TX_MAX: u8 = 0x03;
 
 // Application status words.
 #[repr(u16)]
-#[derive(Clone, Copy, PartialEq)]
+#[derive(Clone, Copy, PartialEq, Debug)]
 pub enum AppSW {
     Deny = 0x6985,
     WrongP1P2 = 0x6A86,
@@ -91,6 +91,7 @@ pub enum AppSW {
     InvalidData = 0x6A80,
     WrongDataLength = 0x6A87,
     WrongResponseLength = 0xB000,
+    InternalError = 0x6F01,
 }
 // To keep consistency with c version app-conflux
 #[allow(dead_code)]

src/settings.rs

@@ -1,3 +1,4 @@
+use crate::AppSW;
 use ledger_device_sdk::nvm::*;
 use ledger_device_sdk::NVMData;
 
@@ -30,20 +31,25 @@ impl Settings {
     }
 
     #[allow(unused)]
-    pub fn get_element(&self, index: usize) -> u8 {
+    pub fn get_element(&self, index: usize) -> Result<u8, AppSW> {
+        if index >= SETTINGS_SIZE {
+            return Err(AppSW::InternalError);
+        }
         let storage = unsafe { DATA.get_ref() };
         let settings = storage.get_ref();
-        settings[index]
+        Ok(settings[index])
     }
 
     #[allow(unused)]
     // can be used to set a value in the settings
-    pub fn set_element(&self, index: usize, value: u8) {
+    pub fn set_element(&self, index: usize, value: u8) -> Result<(), AppSW> {
+        if index >= SETTINGS_SIZE {
+            return Err(AppSW::InternalError);
+        }
         let storage = unsafe { DATA.get_mut() };
         let mut updated_data = *storage.get_ref();
         updated_data[index] = value;
-        unsafe {
-            storage.update(&updated_data);
-        }
+        storage.update(&updated_data);
+        Ok(())
     }
 }

src/types/transaction.rs

@@ -38,30 +38,17 @@ impl Decodable for AccessListItem {
 
 pub type AccessList = Vec<AccessListItem>;
 
-// pub const TX_TYPE_LEGACY: u8 = 0;
-// pub const TX_TYPE_EIP2930: u8 = 1;
-// pub const TX_TYPE_EIP1559: u8 = 2;
-
-// impl Transaction {
-//     pub fn tx_type(&self) -> u8 {
-//         if self.max_fee_per_gas.is_some() && self.max_priority_fee_per_gas.is_some() {
-//             TX_TYPE_EIP1559
-//         } else if self.gas_price.is_some() && self.access_list.is_some() {
-//             TX_TYPE_EIP2930
-//         } else {
-//             // gas_price is required
-//             TX_TYPE_LEGACY
-//         }
-//     }
-// }
+pub const TX_LEGACY_RLP_LEN: usize = 9;
+pub const TX_EIP2930_RLP_LEN: usize = 10;
+pub const TX_EIP1559_RLP_LEN: usize = 11;
 
 impl Decodable for Transaction {
     fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
         if rlp.as_raw().is_empty() {
             return Err(DecoderError::RlpInvalidLength);
         };
         if rlp.is_list() {
-            if rlp.item_count()? != 9 {
+            if rlp.item_count()? != TX_LEGACY_RLP_LEN {
                 return Err(DecoderError::RlpInvalidLength);
             }
             Ok(Transaction {
@@ -87,7 +74,7 @@ impl Decodable for Transaction {
             let rlp = Rlp::new(&data[4..]);
             match first4_bytes {
                 TX_RLP_PREFIX_2930 => {
-                    if rlp.item_count()? != 10 {
+                    if rlp.item_count()? != TX_EIP2930_RLP_LEN {
                         return Err(DecoderError::RlpInvalidLength);
                     }
                     Ok(Transaction {
@@ -106,7 +93,7 @@ impl Decodable for Transaction {
                     })
                 }
                 TX_RLP_PREFIX_1559 => {
-                    if rlp.item_count()? != 11 {
+                    if rlp.item_count()? != TX_EIP1559_RLP_LEN {
                         return Err(DecoderError::RlpInvalidLength);
                     }
                     Ok(Transaction {