Attackathon _ Fuel Network 33175 - [Smart Contract - High] Sway-lib Subtract i Logic Vulnerability

Submitted on Sat Jul 13 2024 12:16:30 GMT-0400 (Atlantic Standard Time) by @Minato7namikazi for Attackathon | Fuel Network

Report ID: #33175

Report type: Smart Contract

Report severity: High

Target: https://github.com/FuelLabs/sway-libs/tree/0f47d33d6e5da25f782fc117d4be15b7b12d291b

Impacts:

• Permanent freezing of funds

Description

Vulnerability Details

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);



}