within sway-libs the subtract method of the Subtract trait implementation for I8
impl core::ops::Subtract for I8 {
/// Subtract a I8 from a I8. Panics of overflow.
fn subtract(self, other: Self) -> Self {
let mut res = Self::new();
if self.underlying >= Self::indent()
&& other.underlying >= Self::indent()
{
if self.underlying > other.underlying {
res = Self::from_uint(self.underlying - other.underlying + Self::indent());
} else {
res = Self::from_uint(self.underlying - (other.underlying - Self::indent()));
}
} else if self.underlying >= Self::indent()
&& other.underlying < Self::indent()
{
res = Self::from_uint(self.underlying - Self::indent() + other.underlying);
} else if self.underlying < Self::indent()
&& other.underlying >= Self::indent()
{
res = Self::from_uint(self.underlying - (other.underlying - Self::indent()));
} else if self.underlying < Self::indent()
&& other.underlying < Self::indent()
{
if self.underlying < other.underlying {
res = Self::from_uint(other.underlying - self.underlying + Self::indent());
} else {
res = Self::from_uint(self.underlying + other.underlying - Self::indent());
}
}
res
}
}
The issue is in the last branch of the conditional statement:
else if self.underlying < Self::indent()
&& other.underlying < Self::indent()
{
if self.underlying < other.underlying {
res = Self::from_uint(other.underlying - self.underlying + Self::indent());
} else {
res = Self::from_uint(self.underlying + other.underlying - Self::indent());
}
}
The problem is in the case where self.underlying < other.underlying. The current implementation is adding Self::indent() instead of subtracting it. This will result in incorrect subtraction for certain negative numbers.
As example to trigger the bug
1- let's creates two I8 values: a representing -2 and b representing -3.
2- let's performs the subtraction a - b, which should equal 1. because -2 - (-3) = 1
and while
a (representing -2) has an underlying value of 126 .. b (representing -3) has an underlying value of 125 .. and Self::indent() is 128
These values fall into the last branch of the Vulnerable if-else statement
else if self.underlying < Self::indent()
&& other.underlying < Self::indent()
{
if self.underlying < other.underlying {
res = Self::from_uint(other.underlying - self.underlying + Self::indent());
} else {
res = Self::from_uint(self.underlying + other.underlying - Self::indent());
}
}
Since 126 (self.underlying) > 125 (other.underlying), it will use the second calculation:
res = Self::from_uint(self.underlying + other.underlying - Self::indent());
Plugging in the values:
res = Self::from_uint(126 + 125 - 128);
res = Self::from_uint(123);
Therefore, the incorrect result will have an underlying value of 123.
Check the PoC for full runnable example
Impact is loss of funds because of
Incorrect Financial Calculations: Any function that uses subtraction with I8 types, especially when dealing with negative numbers, could produce incorrect results. This could lead to logical errors in tokens & funds distribution or any computation that relies on accurate integer arithmetic.
Recommendation
else if self.underlying < Self::indent()
&& other.underlying < Self::indent()
{
if self.underlying < other.underlying {
res = Self::from_uint(Self::indent() - (other.underlying - self.underlying));
} else {
res = Self::from_uint(self.underlying - other.underlying + Self::indent());
}
}
This change ensures that when subtracting a larger negative number from a smaller negative number, the result is correctly calculated as a positive number.
Proof of concept
Add this PoC test in the end of the i8.sw
#[test]
fn i8_Subtraction_PoC() {
// -2 - (-3) should equal 1
// but that isn't true here
let a = I8::neg_from(2u8); // -2
let b = I8::neg_from(3u8); // -3
let result = a - b;
// The expected underlying value for 1 is 129 (128 + 1)
let expected = I8::from_uint(129u8);
// Assert that the underlying values are correct (this should pass)
assert(a.underlying() == 126u8);
assert(b.underlying() == 125u8);
// Assert that the result is not equal to the expected value which prove the bug
assert(!(result == expected));
// the result value will incorrectly calculated as 123u8 instead of 129u8 in this implementation (this should pass)
assert(result.underlying() == 123u8);
}
