Loss of funds due to incorrect rounding

Summary

A user using a JalaMasterRouter will lose unwrapped tokens due to incorrect rounding relative to wrapped tokens.

Vulnerability Detail

JalaMasterRouter is designed to establish the interaction between the wrapped tokens and the JalaRouter02 contract. All underlying unwrapped tokens intreacting with JalaMasterRouter must have 0-17 decimals:

if (_underlyingToken.decimals() >= 18) revert InvalidDecimals();

Using the utils, a wrapped token is created based on underlying token, and decimalsOffset are calculated as follows:

decimalsOffset = 10 ** (18 - _underlyingToken.decimals());

Example: underlying BAR have 0 decimals, decimalsOffset of WBAR(wrapped BAR) will be 10 (18 - _underlyingToken.decimals()), => 10 18. Using removeLiquidityAndUnwrapToken, user transfer liquidity as pair of wrapped tokens and receives unwrapped tokens:

//...
address pair = JalaLibrary.pairFor(factory, wrappedTokenA, wrappedTokenB);
TransferHelper.safeTransferFrom(pair, msg.sender, address(this), liquidity);
//...
if (tokenAReturnAmount > 0) {
            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenA, tokenAReturnAmount); 
}
if (tokenBReturnAmount > 0) {
            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenB, tokenBReturnAmount);
}
//...

Now let's see how a user can lose unwrapped tokens: 1) incorrect rounding: For example, user uses WBAR and WPSG tokens, output tokens BAR and PSG will be as (amountA, amountB):

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

For BAR user will receive tokenAReturnAmount:

uint256 tokenAReturnAmount = (amountA / tokenAOffset) * tokenAOffset;

But if amountA is less than tokenAOffset, for ex. 100 (100 BAR, bc. 0 decimals), tokenAReturnAmount will be 0 due to div. before mul.: tokenAReturnAmount = (100 / (10 * 18)) (10 ** 18) = 0 User will loss 100 BAR (or even more), if tokenAReturnAmount is 0:

if (tokenAReturnAmount > 0) {
            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenA, tokenAReturnAmount); 
}

This issue is mainly related to underlying tokens with small number of decimals.

2) no revert if amount is 0: If tokenAReturnAmount is 0, user will pay amount of wrapped tokens and receive nothing, as there is no revert in code snippet above. Function should revert, if return amount is 0.

3) loss of funds due to wrong variable: Even if tokenAReturnAmount will be greater than 0, it will lose its accuracy due to rounding. Better to use amountA which is more accurate:

-            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenA, tokenAReturnAmount); 
+           IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedTokenA, amountA); 
}

All of this also refers to removeLiquidityETHAndUnwrap and _unwrapAndTransfer.

Impact

Loss of funds due to wrong rounding and calculations.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is problematic to find the best solution when interacting with tokens with such different decimals. Here is one of the ways to resolve this issue:

- uint256 tokenOffset = IChilizWrappedERC20(wrappedToken).getDecimalsOffset();
- uint256 tokenReturnAmount = (amountToken / tokenOffset) * tokenOffset;
- IERC20(wrappedToken).approve(wrapperFactory, tokenReturnAmount); 
+IERC20(wrappedToken).approve(wrapperFactory, amountToken); 
- if (tokenReturnAmount > 0) {
-            IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedToken, tokenReturnAmount); 
- }
+if (amountToken> 0) {
+           IChilizWrapperFactory(wrapperFactory).unwrap(to, wrappedToken, amountToken); 
+} else revert InsufficientAmount();

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

DenTonylifer - Loss of funds due to incorrect rounding #203