Suggest Using Safe Addition and Subtraction Functions for Enhanced Security

[ZigBalthazar]

Description

I have noticed that the current implementation of addition and subtraction functions uses the + and - operators to perform arithmetic operations. While this approach is common, it may lead to potential integer overflow or underflow issues, especially when dealing with large numbers. To enhance the security and robustness of the code, I suggest using the safe addition and subtraction functions provided below(just for example):

// Overflow check for addition (+)
func safeAdd(a, b int64) (int64, error) {
    result := a + b

    if (b > 0 && a > (math.MaxInt64 - b)) || (b < 0 && a < (math.MinInt64 - b)) {
        return 0, fmt.Errorf("Addition resulted in overflow")
    }

    return result, nil
}

// Underflow check for subtraction (-)
func safeSubtract(a, b int64) (int64, error) {
    result := a - b

    if (b > 0 && a < (math.MinInt64 + b)) || (b < 0 && a > (math.MaxInt64 + b)) {
        return 0, fmt.Errorf("Subtraction resulted in underflow")
    }

    return result, nil
}

These safe addition and subtraction functions include checks for potential overflow and underflow scenarios, which help prevent unintended behavior and keep the calculations within the safe bounds of the int64 data type.

By adopting these safe functions in the codebase, we can minimize the risk of unexpected behavior caused by integer overflow or underflow, making the system more reliable and secure, especially in scenarios involving large numeric values.

Thank you for considering this suggestion.

[henrydavies1]

Hello, can I pick this one up?

[b00f]

@ZigBalthazar,

I remember during the early stages of the project, we had an internal discussion about safe operations, such as safeAdd and safeSubtraction.
Generally, there are two approaches here:

1- Check and execute.
2. Check, then execute.

In the first approach (Check and execute), when we want to execute a transaction, we need to ensure that the input data is also valid.
In the second approach (Check, then execute), we check the validity of the data once it is submitted, and during the execution, we just execute it. This approach aligns more with the Single-responsibility principle.

In Pactus, we followed the second approach: check, then execute. If you examine the Pactus code, you can find some basic checks in the SanityCheck functions (like here, here) for basic checks like integer overflow or underflow.

Another issue with safe operations comes down to the preference or taste of developers. Imagine we have a safeAdd method. Then the question arises: where should we use the safeAdd operation? Do we need to use it every time we add two variables, or only for a few specific cases? In the long term, this may make the code less consistent.

At the moment, it is very unlikely that we want to add safe operation methods, even in the util package. However, it's a really good topic to discuss, and I've decided to turn it into a discussion topic to gather more thoughts and ideas.