#37545 [BC-Medium] Deposits with a lock_time of 16 cannot be processed

Submitted on Dec 8th 2024 at 07:54:54 UTC by @PaiMei_and_Gandalf for Attackathon | Stacks

• Report ID: #37545

• Report Type: Blockchain/DLT

• Report severity: Medium

• Target: https://github.com/stacks-network/sbtc/tree/immunefi_attackaton_0.9/sbtc

• Impacts:

  • Direct loss of funds

Description

Brief/Intro

Reclaim scripts that want a lock_time of 16 blocks must be created as the opcode OP_PUSHNUM_16 followed by opcode OP_CSV and followed by the rest of the reclaim script (whatever it may be). It turns out trying to parse a reclaim script with this format throws an error, and deposits with this format cannot be processed by sBTC.

Vulnerability Details

The parsing of a reclaim script in file sbtc/src/deposits.rs matches the script with three patterns, which are the only ones valid for the sBTC project.

    pub fn parse(reclaim_script: &ScriptBuf) -> Result<Self, Error> {
        let (lock_time, script) = match reclaim_script.as_bytes() {
            // These first two branches check for the case when the script
            // is written with as few bytes as possible (called minimal
            // CScriptNum format or something like that).
            [0, OP_CSV, script @ ..] => (0, script),
            // This catches numbers 1-16 and -1. Negative numbers are
            // invalid for OP_CHECKSEQUENCEVERIFY, but we filter them out
            // later in `ReclaimScriptInputs::try_new`.
            [n, OP_CSV, script @ ..]
                if OP_PUSHNUM_NEG1 == *n || (OP_PUSHNUM_1..OP_PUSHNUM_16).contains(n) =>
            {
                (*n as i64 - OP_PUSHNUM_1 as i64 + 1, script)
            }
            // Numbers in bitcoin script are typically only 4 bytes (with a
            // range from -2**31+1 to 2**31-1), unless we are working with
            // OP_CSV or OP_CLTV, where 5-byte numbers are acceptable (with
            // a range of 0 to 2**39-1). See the following for how the code
            // works in bitcoin-core:
            // https://github.com/bitcoin/bitcoin/blob/v27.1/src/script/interpreter.cpp#L531-L573
            // That said, we only accepts 4-byte unsigned integers, and we
            // check that below.
            [n, rest @ ..] if *n <= 5 && rest.get(*n as usize) == Some(&OP_CSV) => {
                // We know the error and panic paths cannot happen because
                // of the above `if` check.
                let (script_num, [OP_CSV, script @ ..]) = rest.split_at(*n as usize) else {
                    return Err(Error::InvalidReclaimScript);
                };
                (read_scriptint(script_num, 5)?, script)
            }
            _ => return Err(Error::InvalidReclaimScript),
        };

        let lock_time =
            u32::try_from(lock_time).map_err(|_| Error::InvalidReclaimScriptLockTime(lock_time))?;

        let script = ScriptBuf::from_bytes(script.to_vec());
        ReclaimScriptInputs::try_new(lock_time, script)
    }

We can see in the second branch of the match statement, this comment:

// This catches numbers 1-16 and -1

...
    [n, OP_CSV, script @ ..]
        if OP_PUSHNUM_NEG1 == *n || (OP_PUSHNUM_1..OP_PUSHNUM_16).contains(n) =>
    {
        (*n as i64 - OP_PUSHNUM_1 as i64 + 1, script)
    }
...

The problem here is OP_PUSHNUM_1..OP_PUSHNUM_16 does not include OP_PUSHNUM_16 in the range, so trying to parse a script with this format will throw an InvalidReclaimScript error.

The fix for this bug is to explicitly include the last element in the range.

...
    [n, OP_CSV, script @ ..]
-        if OP_PUSHNUM_NEG1 == *n || (OP_PUSHNUM_1..OP_PUSHNUM_16).contains(n) =>
+        if OP_PUSHNUM_NEG1 == *n || (OP_PUSHNUM_1..=OP_PUSHNUM_16).contains(n) =>
    {
        (*n as i64 - OP_PUSHNUM_1 as i64 + 1, script)
    }
...

The fact that using a non minimal reclaim script to try to accomplish the same result (having a lock_time of 16 blocks) would parse correctly but would recreate a different reclaim script than the one parsed, would make the function validate_tx also throw an error when trying to validate the transaction:

    pub fn validate_tx(&self, tx: &Transaction) -> Result<DepositInfo, Error> {
...
        // Validate that the deposit and reclaim scripts in the request
        // match the expected formats for deposit transactions.
        let deposit = DepositScriptInputs::parse(&self.deposit_script)?;
        let reclaim = ReclaimScriptInputs::parse(&self.reclaim_script)?;
        // Okay, the deposit and reclaim scripts are valid. Now make sure
        // that the ScriptPubKey in the transaction matches the one implied
        // by the given scripts. So now create the expected ScriptPubKey.
        let deposit_script = deposit.deposit_script();
        let reclaim_script = reclaim.reclaim_script();

        debug_assert_eq!(deposit_script, self.deposit_script);
        debug_assert_eq!(reclaim_script, self.reclaim_script);

        let expected_script_pubkey =
            to_script_pubkey(deposit_script.clone(), reclaim_script.clone());
        // Check that the expected scriptPubkey matches the actual public
        // key of our parsed UTXO.
        if expected_script_pubkey != tx_out.script_pubkey {
            return Err(Error::UtxoScriptPubKeyMismatch(self.outpoint));
        }
...
    }

Because &self.reclaim_script and let reclaim_script = reclaim.reclaim_script(); will end up being different, and then expected_script_pubkey will be different than tx_out.script_pubkey and the validation will throw a UtxoScriptPubKeyMismatch error.

In conclusion, no script with a lock_time of 16 can be used currently in sBTC.

Impact Details

The impact is High because users will lose the fees of the Bitcoin deposit, and the later reclaim of funds for using a valid reclaim script that sBTC simply does not recognize as valid. Also, their funds will be unavailable for them to use for 16 blocks.

References

https://github.com/stacks-network/sbtc/blob/immunefi_attackaton_0.9/sbtc/src/deposits.rs#L446-L485 https://github.com/stacks-network/sbtc/blob/immunefi_attackaton_0.9/sbtc/src/deposits.rs#L126-L172

Proof of Concept

Proof of Concept

We added the following unit test to file sbtc/src/deposits.rs.

    #[test]
    fn OP_PUSHNUM_16_disallowed() {
        let lock_time = 16;

        // The first script is simply the OP_PUSHNUM_16 opcode followed by the OP_CSV opcode
		// It should be parsed correctly, but instead throws an error
        let reclaim_script = ScriptBuf::from_bytes(vec![0x60, 0xB2]);
        let error = ReclaimScriptInputs::parse(&reclaim_script).unwrap_err();

        assert!(matches!(error, Error::InvalidReclaimScript));

        // The second script is 1, followed by the number 16 and followed by the OP_CSV opcode
        // This is a non minimal representation that produces the same script, a lock_time of 16
        // In this case the script is parsed correctly, but when reconstructed it is not equal
        // to reclaim_script2, but to reclaim_script (the one that uses OP_PUSHNUM_16)
        let reclaim_script2 = ScriptBuf::from_bytes(vec![0x01, 0x10, 0xB2]);
        let extracts = ReclaimScriptInputs::parse(&reclaim_script2).unwrap();

        assert_ne!(extracts.reclaim_script(), reclaim_script2);
        assert_eq!(extracts.reclaim_script(), reclaim_script);
    }