Slippage protection will not work when removing liquidity through JalaMasterRouter

Summary

Inside JalaMasterRouter::removeLiquidityAndUnwrapToken after the swap happens additional calculations are made that further reduce the received amount (because of rounding down). However there is no check if the final amount to receive is below the specified minimum. As a result users will experience losses

Vulnerability Detail

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

function removeLiquidityAndUnwrapToken(
        address tokenA, // token origin addr
        address tokenB,
        uint256 liquidity,
        uint256 amountAMin,
        uint256 amountBMin,
        address to,
        uint256 deadline
    ) external virtual override returns (uint256 amountA, uint256 amountB) {
        ....

        (amountA, amountB) = IJalaRouter02(router).removeLiquidity(
            wrappedTokenA,
            wrappedTokenB,
            liquidity,
            amountAMin,
            amountBMin,
            address(this),
            deadline
        );

        uint256 tokenAOffset = IChilizWrappedERC20(wrappedTokenA).getDecimalsOffset();
        uint256 tokenBOffset = IChilizWrappedERC20(wrappedTokenB).getDecimalsOffset();

        uint256 tokenAReturnAmount = (amountA / tokenAOffset) * tokenAOffset;  //@audit -> division before multiplication
        uint256 tokenBReturnAmount = (amountB / tokenBOffset) * tokenBOffset;

        ....
        // NO check is made if returnAmount < amountAMin

        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) {
            TransferHelper.safeTransfer(wrappedTokenA, to, amountA - tokenAReturnAmount);
        }
        if (amountB - tokenBReturnAmount > 0) {
            TransferHelper.safeTransfer(wrappedTokenB, to, amountB - tokenBReturnAmount);
        }
    }

There are two problems in that code that contribute to the bug:

The first one is that division before multiplication is used when calculating the final amounts for tokenAReturnAmount & tokenBReturnAmount. This leads to rounding down and loss of value.
The second one is that there is no check applied after that to ensure that amountAMin >= tokenAReturnAmount && amountBMin>=tokenBReturnAmount. Effectively the slippage protection does not work.

Here is a short example of the bug:

suppose pool exists with wrapped A tokens & B tokens - underlying tokens each have 2 decimals
the market value of a full wrapped token (1e18) is significant, so alice decides she wants to remove only a fraction of each token from the pool
alice calls removeLiquidityAndUnwrapToken providing the appropriate liquidity and setting amountAMin & amountBMin to 0.009e18 (9e15)
The call to the underlying router is successful and 0.009e18 amounts of each token are taken out
One more transformation is made on the amounts, before unwrapping. And this is where loss of value happens
- (amountA / tokenAOffset) * tokenAOffset => (9e15 /1e16) * 1e16 = 0
- (amountA / tokenAOffset) * tokenAOffset => (9e15 /1e16) * 1e16 = 0
But since there is no check with the provided minAmounts, the transaction will not revert and user will receive 0 amounts, while getting his LP tokens burned

Impact

User can loose assets because slippage protection is not applied correctly and division before multiplication is used to calculate the return amount.

Code Snippet

Tool used

Manual Review

Recommendation

It is a general rule in Solidity that division should always come after multiplication. Refactor the calculations inside removeLiquidityAndUnwrapToken & removeLiquidityETHAndUnwrap like so:

// inside removeLiquidityAndUnwrapToken

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

// Inside removeLiquidityETHAndUnwrap
 uint256 tokenReturnAmount = (amountToken * tokenOffset) / tokenOffset;

Also add a slippage check before unwrapping in both functions to ensure they revert and prevent user loss of funds:

require( amountTokenMin >= tokenReturnAmount, "Not enough tokens received")

Additionally the pattern exists in the _unwrapAndTransfer() function as well, so it makes sense to also fix it there:

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

Duplicate of #158

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

b0g0 - Slippage protection will not work when removing liquidity through JalaMasterRouter #182