TOB-FUEL-41: Message proof generation does not validate that block is ahead of genesis

[xgreenx]

Description

The message_proof implementation does not validate that the committed block is greater than the genesis block (zero) prior to subtracting one, resulting in an underflow. While the proof cannot be generated since the committed block is not in the database, this input should be handled with an error as it is elsewhere.

Figure 41.1: Potential overflow of block height (audit-fuel/fuel-core/crates/fuel-core/src/query/message.rs#201)

let verifiable_commit_block_height = *commit_block_header.height() - 1u32.into();

The following test case can be modified to observe the overflow. Note, this test uses a
mocked database but in production that

Figure 41.2: Modifications to existing test to demonstrate overflow

diff --git a/fuel-core/crates/fuel-core/src/query/message/test.rs
b/fuel-core/crates/fuel-core/src/query/message/test.rs
index c351b7c00..662aba5ff 100644
--- a/fuel-core/crates/fuel-core/src/query/message/test.rs
+++ b/fuel-core/crates/fuel-core/src/query/message/test.rs
@@ -83,8 +83,8 @@ mockall::mock! {
 #[tokio::test]
 async fn can_build_message_proof() {
     use mockall::predicate::*;
-    let commit_block_height = BlockHeight::from(2u32);
-    let message_block_height = BlockHeight::from(1u32);
+    let commit_block_height = BlockHeight::from(0u32);
+    let message_block_height = BlockHeight::from(0u32);
     let expected_receipt = receipt(Some(11));
     let message_id = expected_receipt.message_id().unwrap();
     let receipts: [Receipt; 4] = [

Figure 41.3: Implementation of the Sub trait with wrapping semantics (fuel-vm/fuel-types/src/numeric_types.rs#233–240)

impl Sub for $i {
    type Output = $i;
    #[inline(always)]
    fn sub(self, rhs: $i) -> $i {
        $i(self.0.wrapping_sub(rhs.0))
    } 
}

Furthermore, the use of the wrapping_sub and wrabbing_add in the implementation of a Sub and Add trait, respectively, is error-prone as it prevents rustc debug builds from panicking on overflow. This behavior is surprising and masked by being implemented in a macro. It would be ideal to explicitly use wrapping_add as opposed to overriding the semantics of the binary arithmetic operators in debug builds.

Exploit Scenario

A user’s withdrawal proof request is rejected with the wrong error message indicating that the block (the result of the overflow) has not yet been committed rather than indicating the genesis block is an invalid argument. This makes it difficult for the user to withdraw.

Recommendations

Short term, validate that the committed block ID is a predecessor of the genesis block. Do not implement Sub and Add for the BlockId type and instead use checked arithmetic.
Long term, consider setting Clippy’s integer arithmetic lint to deny and use checked, saturating, or wrapping arithmetic explicitly.