Precision loss when calculation tokenOffset

Summary

In the JalaMasterRouter when calculating the tokenReturnAmount in the removeLiquidityAndUnwrapToken function dividing by the tokenAOffset and then multiplying the resulting amount by the tokenAOffset will cause a rounding error and lead to tokenReturnAmount to be zero.

https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol#L101

Vulnerability Detail

The dev already stated in the Discord that the JalaMasterRouter will be used with tokens of decimals from 0-17.

https://discord.com/channels/812037309376495636/1212808646807789669/1214136853930713088

Impact

If a user attempts to remove low amounts of liquidity their tokens will get stuck in the contract. Consider the following POC.

    function test_RemoveLiquidityPOC() public {
        tokenA.approve(address(masterRouter), 10000000);
        tokenB.approve(address(masterRouter), 10000000);

        masterRouter.wrapTokensAndaddLiquidity(
            address(tokenA),
            address(tokenB),
            100,
            100,
            0,
            0,
            user0,
            block.timestamp
        );

        address wrappedTokenA = wrapperFactory.wrappedTokenFor(address(tokenA));
        address wrappedTokenB = wrapperFactory.wrappedTokenFor(address(tokenB));
        address pairAddress = factory.getPair(address(wrappedTokenA), address(wrappedTokenB));
        JalaPair pair = JalaPair(pairAddress);
        uint256 liquidity = pair.balanceOf(user0);

        vm.startPrank(user0);
        pair.approve(address(masterRouter), liquidity);

        console.log("balance of lp tokens before", pair.balanceOf(user0));
        console.log("balace of tokenA", tokenA.balanceOf(user0));
        console.log("wrapped tokne balance before", IERC20(wrappedTokenA).balanceOf(user0));
        uint decimals = tokenA.decimals();
        console.log("decimals", decimals);

        masterRouter.removeLiquidityAndUnwrapToken(
            address(tokenA),
            address(tokenB),
            1000,
            0,
            0,
            user0,
            block.timestamp
        );
        IERC20(wrappedTokenA).approve(address(wrapperFactory), 100);
        IChilizWrapperFactory(wrapperFactory).unwrap(user0, wrappedTokenA, 1);
        vm.stopPrank();

        console.log("abalcd lp tokens after", pair.balanceOf(user0));
        console.log("balance of token a after", tokenA.balanceOf(user0));
        console.log("wrapped token balance after", IERC20(wrappedTokenA).balanceOf(user0));
    }

  balance of lp tokens before 3162277660168378331
  balace of tokenA 10000
  wrapped tokne balance before 0
  decimals 3
  amounts 31 31622
  amountA 31
  tokenAOffset 1000000000000000
  tokenAReturnAmount 0
  wrappedAmount 31
  Balance lp tokens after 3162277660168377331
  balance of token a after 10001
  wrapped token balance after 30

Of course we're assuming that the user puts a low amount of slippage, in this example we used 0 slippage however this will work with other amounts such as 1 or 2.

We can see from the results that tokenAReturnAmount returns 0. Inherently we would think this is fine because the following line would cause these tokens to be sent as wrapped tokens and the only issue is the user would have to pay for an extra transaction to unwrap them. The problem here is for tokens with small amounts and large decimalsOffset the user would be unable to unwrap them as there is also a precision loss issue in the withdrawTo function in the ChilizWrappedERC20 contract. Leaving those tokens stuck in the contract.

Code Snippet

Below you can see where the issue resides, dividing before multiple causes the rounding issue making tokenAReturnAmount and tokenBRetunAmount to round down to zero.

        uint256 tokenAReturnAmount = (amountA / tokenAOffset) * tokenAOffset;
        uint256 tokenBReturnAmount = (amountB / tokenBOffset) * tokenBOffset;

This would then cause the code block below to transfer these tokens as wrapped tokens.

        if (tokenAReturnAmount > 0) {
            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenA, tokenAReturnAmount);
        }
        if (tokenBReturnAmount > 0) {
            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenB, tokenBReturnAmount);
        }
        // transfer dust as wrapped token
        if (amountA - tokenAReturnAmount > 0) {
            uint wrappedAmount = amountA - tokenAReturnAmount;
            console.log("wrappedAmount", wrappedAmount);
            TransferHelper.safeTransfer(wrappedTokenA, to, amountA - tokenAReturnAmount);
        }
        if (amountB - tokenBReturnAmount > 0) {
            TransferHelper.safeTransfer(wrappedTokenB, to, amountB - tokenBReturnAmount);
        }

As per the other issue that with precision loss in the withdrawTo function if these token amounts are small they will get stuck in the wrapped contract and a user would not be able to unwrap them.


    function withdrawTo(address account, uint256 amount) public virtual returns (bool) {
        if (address(underlyingToken) == address(0)) revert NotInitialized();
        uint256 unwrapAmount = amount * decimalsOffset;
        if (unwrapAmount == 0) revert CannotWithdraw();
        address msgSender = _msgSender();
        uint256 burntAmount = unwrapAmount / decimalsOffset;
        _burn(msgSender, burntAmount);
        SafeERC20.safeTransfer(underlyingToken, account, amount);
        if (msgSender != account) {
            _transfer(msgSender, account, amount - burntAmount);
        }

        emit Withdraw(account, amount);

        return true;
    }

Tool used

Manual Review

Recommendation

Consider multiplying before dividing to avoid precision loss issues.

        uint256 tokenAReturnAmount = (amountA * tokenAOffset) / tokenAOffset;
        uint256 tokenBReturnAmount = (amountB * tokenBOffset) / tokenBOffset;

Duplicate of #158

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

33audits - Precision loss when calculation tokenOffset #235