solana: Configure clippy to deny possible truncation (#285)

* solana: Configure clippy to deny possible truncation

Adds a clippy rule to block the inclusion of code that can result in
truncation (i.e. `as T` conversions)
Adds linting directive to existing instances in the code and add
comments about why they are safe
Add a unit test on bitmap length to confirm that the maximum possible
number of votes (i.e. maximum "length" of the bitmap) is within the
limits of u8.

* solana: deny truncation in Cargo.toml instead of GitHub yml

* solana: Remove TODO referencing old PRs

* node: solana CI - fix Cargo.toml path

* solana: Change "as" to try_from()

Author: johnsaigle

.github/workflows/solana.yml

@@ -50,7 +50,7 @@ jobs:
         run: cargo check --workspace --tests --manifest-path Cargo.toml
 
       - name: Run `cargo clippy`
-        run: cargo clippy --workspace --tests --manifest-path Cargo.toml
+        run: cargo clippy --workspace --tests --manifest-path Cargo.toml -- -Dclippy::cast_possible_truncation
 
       - name: Cache solana tools
         id: cache-solana

solana/Cargo.toml

@@ -10,9 +10,8 @@ warnings = "deny"
 
 # Lints to improve security
 [workspace.lints.clippy]
-# The following rules should be enabled but conflict with other open PRs.
 # TODO enable the commented rules below
-# cast_possible_truncation = "deny"
+cast_possible_truncation = "deny"
 # cast_lossless= "deny"
 # as_conversions = "deny"
 # arithmetic_side_effects = "deny"

solana/programs/example-native-token-transfers/src/bitmap.rs

@@ -43,8 +43,6 @@ impl Bitmap {
 
     pub fn count_enabled_votes(&self, enabled: Bitmap) -> u8 {
         let bm = BM::<128>::from_value(self.map) & BM::<128>::from_value(enabled.map);
-        // Conversion from usize to u8 is safe here. The Bitmap uses u128, so its maximum length
-        // (number of true bits) is 128.
         bm.len()
             .try_into()
             .expect("Bitmap length must not exceed the bounds of u8")

solana/programs/example-native-token-transfers/src/queue/rate_limit.rs

@@ -46,12 +46,15 @@ impl RateLimitState {
     /// Returns the capacity of the rate limiter.
     /// On-chain programs and unit tests should always use [`capacity`].
     /// This function is useful in solana-program-test, where the clock sysvar
-    /// SECURITY: Integer division is OK here. We are not that concerned with precision. Removing
-    /// the remainder in this case is arguably more secure as it reduces the available capacity.
-    /// SECURITY: Sign loss is OK here. It is a conversion performed on a timestamp that must always be
-    /// positive.
+    // SECURITY: Integer division is OK here. We are not that concerned with precision. Removing
+    // the remainder in this case is arguably more secure as it reduces the available capacity.
+    // SECURITY: Sign loss is OK here. It is a conversion performed on a timestamp that must always be
+    // positive.
+    // SECURITY: Truncation is allowed here. Clippy warns about the final returned expression, but it is
+    // safe.
     #[allow(clippy::integer_division)]
     #[allow(clippy::cast_sign_loss)]
+    #[allow(clippy::cast_possible_truncation)]
     pub fn capacity_at(&self, now: UnixTimestamp) -> u64 {
         assert!(self.last_tx_timestamp <= now);
 
@@ -76,6 +79,10 @@ impl RateLimitState {
                 + time_passed as u128 * limit / (Self::RATE_LIMIT_DURATION as u128)
         };
 
+        // The use of `min` here prevents truncation.
+        // The value of `limit` is u64 in reality. If both `calculated_capacity` and `limit` are at
+        // their maxiumum possible values (u128::MAX and u64::MAX), then u64::MAX will be chosen by
+        // `min`. So truncation is not possible.
         calculated_capacity.min(limit) as u64
     }
 

solana/programs/wormhole-governance/src/instructions/governance.rs

@@ -162,7 +162,9 @@ impl GovernanceMessage {
             acc.is_signer.write(writer)?;
             acc.is_writable.write(writer)?;
         }
-        (data.len() as u16).write(writer)?;
+        u16::try_from(data.len())
+            .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "data length overflow"))?
+            .write(writer)?;
         writer.write_all(data)
     }
 }