Loss of precission when unwrapping small amounts of tokens

Summary

In the ChilizWrappedERC20 when calculating the amount to unwrap in the withdrawTo function dividing by the decimalOffset and then multiplying the resulting amount by the decimalOffset will cause small amounts of tokens to get stuck in the contract and won't allow a user to unwrap these amounts.

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

Vulnerability Detail

The following proof of concept wraps 100 wei of tokenA which in this example is a token with 3 decimals leaving the tokenOffset to be 10 ** 15.

You can see below how the ChilizWrappedERC20 contract calculates the tokenOffset. This is the first part of the problem. Due to this strange calculation it will divided the amount to unwrap by a number which is 10 **15 and is much larger then the actual decimals being passed for the underlying token.

        if (msg.sender != factory) revert Forbidden();
        if (_underlyingToken.decimals() >= 18) revert InvalidDecimals();
        if (address(underlyingToken) != address(0)) revert AlreadyExists();

        underlyingToken = _underlyingToken;
        decimalsOffset = 10 ** (18 - _underlyingToken.decimals());
        name = string(abi.encodePacked("Wrapped ", _underlyingToken.name()));
        symbol = string(abi.encodePacked("W", _underlyingToken.symbol()));
    }

Below we can see a POC which attempts to wrap 100 tokens and then unwrap it except when unwrapping due to rounding errors it will round down to zero and the CannotWithdraw() error will be thrown.

tokenA = new ERC20Mintable("Token A", "TKNA", 3);

 function test_WrapAndUnwrapToken() public{
        tokenA.approve(address(wrapperFactory), type(uint256).max);
        wrapperFactory.wrap(address(this), address(tokenA), 100);
        address wrappedTokenA = wrapperFactory.wrappedTokenFor(address(tokenA));
        uint decimals = tokenA.decimals();
        console.log("decimals", decimals);
        IERC20(wrappedTokenA).approve(address(wrapperFactory), 100);
        wrapperFactory.unwrap(address(this), wrappedTokenA, 100);

    }

Below are the results from running the POC with a small amount of wei.

Ran 1 test suite in 1.24s (2.59ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/JalaMasterRouter.t.sol:JalaMasterRouter_Test
[FAIL. Reason: CannotWithdraw()] test_WrapAndUnwrapToken() (gas: 1938038)

Impact

Tokens with small decimals greater than zero or small token amounts will get stuck in the contract.

Code Snippet

You can see in the code snippet below where dividing by tokenOffset first will cause the value of amount to round down if it is less then the tokenOffset being used.

    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, unwrapAmount);
        if (msgSender != account) {
            _transfer(msgSender, account, amount - burntAmount);
        }

        emit Withdraw(account, amount);

        return true;
    }

Tool used

Manual Review

Recommendation

The withdrawTo function is overly complex and could be simplified or rewritten to avoid this type of issue. Below is a fix that multiples before dividing which allows the user to be able to withdraw even small amounts of wrapped tokens.

 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;
    }

Alternatively you could use solmate or solady fixedpoint libraries to better handle decimals and rounding errors. Using wad for greater precision could help avoid these issues as well.

Duplicate of #104

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

33audits - Loss of precission when unwrapping small amounts of tokens #229