fix(tariscript): protect compare and check height from underflows (#5872

)

Description
---
This PR corrects the operand position of the `handle_compare_height`
function. This was identified as incorrect based on the op code
description.
Additionally, protect it from underflows, and write test cases against
all possible returns and errors.

I added underflow protection in the `handle_check_height` function as
well but after some thought this function wouldn't currently be possible
to underflow. The values passed into the function would never be lower
than 0, and then converted to i64 which would handle subtraction from 0
fine. In opposition, the `handle_compare_height` uses a value from the
stack that could be i64::MIN and then have i64::MAX subtracted from it.
Which was the originally identified problem. This is all to say the
functions aren't quite equal so it felt worth a comment for future
readers.

Motivation and Context
---
Closes #5813 

How Has This Been Tested?
---
Added new tests.

What process can a PR reviewer use to test or verify this change?
---
Double check the
[docs](https://github.com/tari-project/tari/blob/development/infrastructure/tari_script/src/op_codes.rs#L146)
to validate the operand switch is a valid change.

See the new underflow protection. 

Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

infrastructure/tari_script/src/error.rs

@@ -49,6 +49,8 @@ pub enum ScriptError {
     VerifyFailed,
     #[error("as_hash requires a Digest function that returns at least 32 bytes")]
     InvalidDigest,
+    #[error("A compare opcode failed, aborting the script immediately with reason: `{0}`")]
+    CompareFailed(String),
 }
 
 impl From<TryFromIntError> for ScriptError {

infrastructure/tari_script/src/op_codes.rs

@@ -143,7 +143,7 @@ pub enum Opcode {
     /// if there is not a valid integer value on top of the stack. Fails with `StackUnderflow` if the stack is empty.
     /// Fails with `VerifyFailed` if the block height < `height`.
     CompareHeightVerify,
-    /// Pops the top of the stack as `height`, then pushes the value of (`height` - the current height) to the stack.
+    /// Pops the top of the stack as `height`, then pushes the value of (the current height - `height`) to the stack.
     /// In other words, this opcode replaces the top of the stack with the difference between `height` and the
     /// current height. Fails with `InvalidInput` if there is not a valid integer value on top of the stack. Fails
     /// with `StackUnderflow` if the stack is empty.

infrastructure/tari_script/src/script.rs

@@ -340,7 +340,18 @@ impl TariScript {
     fn handle_check_height(stack: &mut ExecutionStack, height: u64, block_height: u64) -> Result<(), ScriptError> {
         let height = i64::try_from(height)?;
         let block_height = i64::try_from(block_height)?;
-        let item = StackItem::Number(block_height - height);
+
+        // Due to the conversion of u64 into i64 which would fail above if they overflowed, these
+        // numbers should never enter a state where a `sub` could fail. As they'd both be within range and 0 or above.
+        // This differs from compare_height due to a stack number being used, which can be lower than 0
+        let item = match block_height.checked_sub(height) {
+            Some(num) => StackItem::Number(num),
+            None => {
+                return Err(ScriptError::CompareFailed(
+                    "Subtraction of given height from current block height failed".to_string(),
+                ))
+            },
+        };
 
         stack.push(item)
     }
@@ -359,7 +370,16 @@ impl TariScript {
         let target_height = stack.pop_into_number::<i64>()?;
         let block_height = i64::try_from(block_height)?;
 
-        let item = StackItem::Number(block_height - target_height);
+        // Here it is possible to underflow because the stack can take lower numbers where check
+        // height does not use a stack number and it's minimum can't be lower than 0.
+        let item = match block_height.checked_sub(target_height) {
+            Some(num) => StackItem::Number(num),
+            None => {
+                return Err(ScriptError::CompareFailed(
+                    "Couldn't subtract the target height from the current block height".to_string(),
+                ))
+            },
+        };
 
         stack.push(item)
     }
@@ -1727,4 +1747,113 @@ mod test {
         let buf = &mut buf.as_slice();
         assert!(TariScript::deserialize(buf).is_err());
     }
+
+    #[test]
+    fn test_compare_height_block_height_exceeds_bounds() {
+        let script = script!(CompareHeight);
+
+        let inputs = inputs!(0);
+        let ctx = context_with_height(u64::MAX);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds)));
+    }
+
+    #[test]
+    fn test_compare_height_underflows() {
+        let script = script!(CompareHeight);
+
+        let inputs = ExecutionStack::new(vec![Number(i64::MIN)]);
+        let ctx = context_with_height(i64::MAX as u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::CompareFailed(_))));
+    }
+
+    #[test]
+    fn test_compare_height_underflows_on_empty_stack() {
+        let script = script!(CompareHeight);
+
+        let inputs = ExecutionStack::new(vec![]);
+        let ctx = context_with_height(i64::MAX as u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::StackUnderflow)));
+    }
+
+    #[test]
+    fn test_compare_height_valid_with_uint_result() {
+        let script = script!(CompareHeight);
+
+        let inputs = inputs!(100);
+        let ctx = context_with_height(24_u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(stack_item.is_ok());
+        assert_eq!(stack_item.unwrap(), Number(-76))
+    }
+
+    #[test]
+    fn test_compare_height_valid_with_int_result() {
+        let script = script!(CompareHeight);
+
+        let inputs = inputs!(100);
+        let ctx = context_with_height(110_u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(stack_item.is_ok());
+        assert_eq!(stack_item.unwrap(), Number(10))
+    }
+
+    #[test]
+    fn test_check_height_block_height_exceeds_bounds() {
+        let script = script!(CheckHeight(0));
+
+        let inputs = ExecutionStack::new(vec![]);
+        let ctx = context_with_height(u64::MAX);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds)));
+    }
+
+    #[test]
+    fn test_check_height_exceeds_bounds() {
+        let script = script!(CheckHeight(u64::MAX));
+
+        let inputs = ExecutionStack::new(vec![]);
+        let ctx = context_with_height(10_u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds)));
+    }
+
+    #[test]
+    fn test_check_height_overflows_on_max_stack() {
+        let script = script!(CheckHeight(0));
+
+        let mut inputs = ExecutionStack::new(vec![]);
+
+        for i in 0..255 {
+            inputs.push(Number(i)).unwrap();
+        }
+
+        let ctx = context_with_height(i64::MAX as u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(matches!(stack_item, Err(ScriptError::StackOverflow)));
+    }
+
+    #[test]
+    fn test_check_height_valid_with_uint_result() {
+        let script = script!(CheckHeight(24));
+
+        let inputs = ExecutionStack::new(vec![]);
+        let ctx = context_with_height(100_u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(stack_item.is_ok());
+        assert_eq!(stack_item.unwrap(), Number(76))
+    }
+
+    #[test]
+    fn test_check_height_valid_with_int_result() {
+        let script = script!(CheckHeight(100));
+
+        let inputs = ExecutionStack::new(vec![]);
+        let ctx = context_with_height(24_u64);
+        let stack_item = script.execute_with_context(&inputs, &ctx);
+        assert!(stack_item.is_ok());
+        assert_eq!(stack_item.unwrap(), Number(-76))
+    }
 }