sherlock-audit / 2024-02-jala-swap-judging

6 stars 4 forks source link

0x996 - Due to precision loss, it is impossible to correctly calculate `tokenAReturnAmount`, `tokenBReturnAmount`, `tokenReturnAmount`, and `tokenOutReturnAmount`. #184

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 8 months ago

0x996

medium

Due to precision loss, it is impossible to correctly calculate tokenAReturnAmount, tokenBReturnAmount, tokenReturnAmount, and tokenOutReturnAmount.

Summary:

Due to precision loss, it is impossible to correctly calculate tokenAReturnAmount, tokenBReturnAmount, tokenReturnAmount, and tokenOutReturnAmount.

Vulnerability Detail

  1. The reason lies in the fact that the calculation of these values uses Divide before multiply, where performing division before multiplication leads to precision loss.
  2. A simple example is as follows (can be run in Remix):
    
    // SPDX-License-Identifier: GPL-3.0
    pragma solidity ^0.8.0;

contract test { uint256 a; uint256 b;

constructor(uint256 _a, uint256 _b) {
    a = _a;
    b = _b;
}

function test01() public view returns (uint256){
    return (a * b) / b;
}

function test02() public view returns (uint256){
    return (a / b) * b;
}

}

// The final result: // a = 1, b =2: // Call test01 -->> Result: 1 // Call test02 -->> Result: 0


## Impact

precision loss

## Code Snippet:

- [https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol#L129-L130](https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol#L129-L130)
- https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol#L175
- https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol#L312

## Tool used

Manual Review

## Recommendation:

Consider ordering multiplication before division.

```diff
-    uint256 tokenAReturnAmount = (amountA / tokenAOffset) * tokenAOffset;
-    uint256 tokenBReturnAmount = (amountB / tokenBOffset) * tokenBOffset;
+    uint256 tokenAReturnAmount = (amountA * tokenAOffset) / tokenAOffset;
+    uint256 tokenBReturnAmount = (amountB * tokenBOffset) / tokenBOffset;
.
.
.
-     uint256 tokenReturnAmount = (amountToken / tokenOffset) * tokenOffset;
+     uint256 tokenReturnAmount = (amountToken * tokenOffset) / tokenOffset;
.
.
.
-     uint256 tokenOutReturnAmount = (balanceOut / tokenOutOffset) * tokenOutOffset;
+     uint256 tokenOutReturnAmount = (balanceOut * tokenOutOffset) / tokenOutOffset;
nevillehuang commented 7 months ago

Invalid, lack detailed numerical example/PoC and description of impact required by sherlock