[Feature]: Reclaim script lock time check can be bypassed

[evonide]

(Medium) Reclaim script lock time check can be bypassed

1. Description

The code currently parses Bitcoin TX reclaim_scripts for deposit requests in sbtc/sbtc/src/deposits.rs at 033e2cbd2def829a3e30df77bc274b325d68cdbe · stacks-network/sbtc. The reclaim script’s idea is that a user should be able to reclaim their UTXO after a certain time period. To do so the parsing checks the reclaim format against the following ones: https://github.com/stacks-network/sbtc/blob/ffbe79e0238825f3546e9429bbf89042703823df/sbtc/src/deposits.rs#L421

    pub fn parse(reclaim_script: &ScriptBuf) -> Result<Self, Error> {
        let (lock_time, script) = match reclaim_script.as_bytes() {
            [0, OP_CSV, script @ ..] [...]
            [n, OP_CSV, script @ ..] [...]
            [n, rest @ ..] [...]
            [...]
    }

There are some constraints on this provided lock_time such as: https://github.com/stacks-network/sbtc/blob/ffbe79e0238825f3546e9429bbf89042703823df/sbtc/src/deposits.rs#L386-L389

However, this does not enforce a minimum locking time. Accordingly, a format such as [0, OP_CSV, script @ ..] should allow a user to currently effectively bypass the lock time check. Additionally, as per Locktime | Post-dating a Bitcoin Transaction

[...] If all of the inputs' sequence values are set to the maximum value of 0xffffffff, the transaction is considered "final" and the locktime feature is disabled.

• There’s doesn't seem to be a check against this edge case which effectively would provide another way to bypass the time lock mechanism.
• Please note hat the Bitcoin code seems to bypass the locktime feature once the MSB at 1 << 31 is set as per https://github.com/bitcoin/bitcoin/blob/349632e022da22a457a85650360b5be41fa500dc/src/script/interpreter.cpp#L584. This might need some closer observation to implement securely.

As per [Design]: sBTC Deposit UTXO Binary Format · Issue #30 · stacks-network/sbtc:

The timelock allow Signers to choose to reject transactions if there is a risk that a user may attempt reclaim the deposit UTXO in the same block as the sBTC transaction which would consume the same UTXO, which would increase the risk of the sBTC transaction not being included in a block.

This issue may not have a severe impact beyond wasting gas for signers. However, a minimum lock time check should be enforced, and the sequence value should be verified to prevent users from being able to reclaim the UTXO prematurely.

[djordon]

However, this does not enforce a minimum locking time. Accordingly, a format such as [0, OP_CSV, script @ ..] should allow a user to currently effectively bypass the lock time check.

Yeah, this is good to note.

1. We haven't finalized what the minimum lock-time will be (yeah we should do that). But even if we did, I would think that we should only check for the "absolute minimum" since the minimum can change. So this checks that the deposit matches what the depositor claimed and that it has the correct format.
2. The plan is for the signers to only sweep in deposit transactions with enough "time left" on them. So if a new deposit transaction has a OP_CSV time of "four" (say) then the deposit will be treated the same as if their OP_CSV time was originally set to 1000 but it now has "four" OP_CSV time left.

Anyway, that's how I view it. Do you think that we should change it with the above in mind?

the sequence value should be verified to prevent users from being able to reclaim the UTXO prematurely.

Are you sure? I think all of that was referring to the transaction-level locktime, (which is sometimes referred to as the nLocktime), but we are using the OP_CSV lock time.

[evonide]

Thanks Daniel,

1. We haven't finalized what the minimum lock-time will be (yeah we should do that). But even if we did, I would think that we should only check for the "absolute minimum" since the minimum can change. So this checks that the deposit matches what the depositor claimed and that it has the correct format.
2. The plan is for the signers to only sweep in deposit transactions with enough "time left" on them. So if a new deposit transaction has a OP_CSV time of "four" (say) then the deposit will be treated the same as if their OP_CSV time was originally set to 1000 but it now has "four" OP_CSV time left.

Anyway, that's how I view it. Do you think that we should change it with the above in mind?

That makes sense and should help avoid premature reclaims.

Are you sure? I think all of that was referring to the transaction-level locktime, (which is sometimes referred to as the nLocktime), but we are using the OP_CSV lock time.

There are two operands OP_CHECKLOCKTIMEVERIFY (OP_CLTV) and OP_CHECKSEQUENCEVERIFY (OP_CSV). Indeed, the statements about nLockTime rather apply to OP_CLTV (as per https://github.com/BlockchainCommons/Learning-Bitcoin-from-the-Command-Line/blob/master/11_2_Using_CLTV_in_Scripts.md#understand-how-cltv-really-works) which uses an absolute timelock.

OP_CSV uses the relative timelock nSequence (https://github.com/BlockchainCommons/Learning-Bitcoin-from-the-Command-Line/blob/master/11_3_Using_CSV_in_Scripts.md#summary-using-csv-in-scripts):

However, note that there seem to be some pitfalls here:

The 32nd bit is used to positively signal if relative timelocks are deactivated.

All you have to do is issue a transaction where the nSequence in an input is set as shown above: with the nSequence for that input set such that the first two bytes define the timelock, the 23rd bit defines the type of timelock, and the 32nd bit is set to false..

The source of truth should be the Bitcoin code as referenced in https://github.com/bitcoin/bitcoin/blob/349632e022da22a457a85650360b5be41fa500dc/src/script/interpreter.cpp#L584. You can see that if the 32nd bit is set the check will just be skipped.

Please resolve this issue if you feel the code already takes these edge cases correctly into account of course.

[evonide]

@djordon After reviewing the fix there might still be a few potentially concerning issues that need investigation. I would suggest to reopen this issue for now.

1) Regarding Bit 23 of the provided lock_time:

The 23rd bit is used to positively signal if the lock refers to a time rather than a blockheight.

The code does not seem to currently check for this. If the bit is NOT set this would just use the provided value to measure if sufficiently many seconds have passed.

2) More critically:

The nVersion field must be set to 2 or more.

Currently, I can't find a check for this in the code. If this is being left out the feature won't work at all and can be easily bypassed by specifying version 1 in your transaction. See https://en.bitcoin.it/wiki/Transaction#General_format_of_a_Bitcoin_transaction_(inside_a_block).

Since this manual parsing is very error prone and hard to get right it might be worth investigating using a library such as https://github.com/rust-bitcoin/rust-bitcoin.

[djordon]

Currently, I can't find a check for this in the code. If this is being left out the feature won't work at all and can be easily bypassed by specifying version 1 in your transaction. See https://en.bitcoin.it/wiki/Transaction#General_format_of_a_Bitcoin_transaction_(inside_a_block).

Oh my! Thanks for highlighting!

1. Regarding Bit 23 of the provided lock_time:

The 23rd bit is used to positively signal if the lock refers to a time rather than a blockheight.

Yeah we don't handle this yet, we have https://github.com/stacks-network/sbtc/issues/380 for handling it correctly. But I should flesh out that description so that it's clearer what needs to happen.

[djordon]

Since this manual parsing is very error prone and hard to get right it might be worth investigating using a library such as https://github.com/rust-bitcoin/rust-bitcoin.

I first attempted to do parsing using that library, but it was harder than doing it manually. I'll revisit this to see if I was just misunderstanding something.

[djordon]

Since this manual parsing is very error prone and hard to get right it might be worth investigating using a library such as https://github.com/rust-bitcoin/rust-bitcoin.

Oh you mean to use this stuff here: https://docs.rs/bitcoin/0.32.3/bitcoin/locktime/relative/index.html. Yeah great point, will do. Thank you.

[djordon]

3. More critically:

The nVersion field must be set to 2 or more.

Currently, I can't find a check for this in the code. If this is being left out the feature won't work at all and can be easily bypassed by specifying version 1 in your transaction. See https://en.bitcoin.it/wiki/Transaction#General_format_of_a_Bitcoin_transaction_(inside_a_block).

Well, some good news, sorta. After reading through the BIP-112 again it seems like we do not need to have a check for the version. When the depositor attempts to reclaim their deposit, they must make sure to that they set the transaction version to 2 or else the script will fail. The signers don't need to do such a check for their transaction (well at least not because of OP_CSV).

I checked the source and seems to check out. I looked at https://github.com/bitcoin/bitcoin/blob/v27.1/src/script/interpreter.cpp#L1744-L1747 and https://github.com/bitcoin/bitcoin/blob/v27.1/src/script/interpreter.cpp#L560-L592.

@evonide does all of this make sense to you or am I mistaken?

[evonide]

Agreed, thanks for checking this in more detail Daniel!

It's quite confusing as the reference implementation in https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki seems to fail open:

    // tx.nVersion is signed integer so requires cast to unsigned otherwise
    // we would be doing a signed comparison and half the range of nVersion
    // wouldn't support BIP 68.
    bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2
                      && flags & LOCKTIME_VERIFY_SEQUENCE;

    // Do not enforce sequence numbers as a relative lock time
    // unless we have been instructed to
    if (!fEnforceBIP68) {
        return std::make_pair(nMinHeight, nMinTime);
    }

However, reading the linked BIP-112:

When executed, if any of the following conditions are true, the script interpreter will terminate with an error:
[...]
the top item on the stack has the disable flag (1 << 31) unset; and
the transaction version is less than 2; or
[...]

and according to the linked Bitcoin code:

   case OP_CHECKSEQUENCEVERIFY:
                {
[...]
                    // Compare the specified sequence number with the input.
                    if (!checker.CheckSequence(nSequence))
                        return set_error(serror, SCRIPT_ERR_UNSATISFIED_LOCKTIME);
[...]
                }

and

template <class T>
bool GenericTransactionSignatureChecker<T>::CheckSequence(const CScriptNum& nSequence) const
{
[...]
    // Fail if the transaction's version number is not set high
    // enough to trigger BIP 68 rules.
    if (static_cast<uint32_t>(txTo->nVersion) < 2)
        return false;
[...]
}

This fails closed and I concur with your assessment which is good to see. In this case, we can resolve this issue once the bit 23 aspect is addressed.