From 91823e27dd8c53725614708b2ad36b5fad7ff87c Mon Sep 17 00:00:00 2001 From: tomasavola <108414862+tomasavola@users.noreply.github.com> Date: Wed, 7 Aug 2024 09:44:24 -0300 Subject: [PATCH 1/2] New divide-before-multiply documentation --- .../detectors/1-divide-before-multiply.md | 64 +++++++++++-------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/docs/docs/detectors/1-divide-before-multiply.md b/docs/docs/detectors/1-divide-before-multiply.md index 54d4f3c7..5bf96624 100644 --- a/docs/docs/detectors/1-divide-before-multiply.md +++ b/docs/docs/detectors/1-divide-before-multiply.md @@ -1,43 +1,55 @@ # Divide before multiply -### What it does +## Description -Checks the existence of a division before a multiplication. +- Category: `Arithmetic` +- Severity: `Medium` +- Detectors: [`divide-before-multiply`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply) +- Test Cases: [`divide-before-multiply-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1) [`divide-before-multiply-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-2) [`divide-before-multiply-3`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-3) + +In Rust, the order of operations can influence the precision of the result, especially in integer arithmetic. -### Why is this bad? +## Why is this bad? -Performing a division operation before multiplication can lead to a loss of precision. It might even result in an unintended zero value. +Performing a division operation before a multiplication can lead to a loss of precision as division between integers might return zero. It might even result in an unintended zero value. -### Example +## Issue example + +Consider the following `Soroban` contract: ```rust -// Example 1 - Vulnerable -let x = 10; -let y = 6; -let z = x / y * 3; // z evaluates to 3 - -// Example 2 - Vulnerable -let a = 1; -let b = 2; -let c = a / b * 3; // c evaluates to 0 + + pub fn split_profit(percentage: u64, total_profit: u64) -> u64 { + (percentage / 100) * total_profit +} + ``` -Use instead: +In this contract, the `split_profit` function divides the `percentage` by `100` before multiplying it with `total_profit`. This could lead to a loss of precision if `percentage` is less than `100` as the division would return `0`. This could lead to incorrect calculations and potential financial loss in a real-world smart contract. + + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1/vulnerable-example). + + +## Remediated example + +Reverse the order of operations to ensure multiplication occurs before division. ```rust -// Example 1 - Remediated -let x = 10; -let y = 6; -let z = x * 3 / y; // z evaluates to 5 - -// Example 2 - Remediated -let a = 1; -let b = 2; -let c = a * 3 / b; // c evaluates to 1 + + pub fn split_profit(&self, percentage: u64, total_profit: u64) -> u64 { + (percentage * total_profit) / 100 + } + ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/divide-before-multiply/divide-before-multiply-1/remediated-example). + +## How is it detected? -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/divide-before-multiply). +Checks the existence of a division before a multiplication. +## References +[Rust documentation: `Integer Division`](https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators) + From d026c07ab87437e5809663640953ab1ad53d2ef3 Mon Sep 17 00:00:00 2001 From: tomasavola <108414862+tomasavola@users.noreply.github.com> Date: Wed, 7 Aug 2024 12:14:30 -0300 Subject: [PATCH 2/2] Update "Why is this bad?" --- docs/docs/detectors/1-divide-before-multiply.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/detectors/1-divide-before-multiply.md b/docs/docs/detectors/1-divide-before-multiply.md index 5bf96624..5d5f5367 100644 --- a/docs/docs/detectors/1-divide-before-multiply.md +++ b/docs/docs/detectors/1-divide-before-multiply.md @@ -11,7 +11,7 @@ In Rust, the order of operations can influence the precision of the result, espe ## Why is this bad? -Performing a division operation before a multiplication can lead to a loss of precision as division between integers might return zero. It might even result in an unintended zero value. +Performing a division operation before a multiplication can lead to a loss of precision as division between integers might return zero. ## Issue example