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()

.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)
  }
 }