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

6 stars 4 forks source link

giraffe - Precision loss in multiple areas of MasterRouter #255

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

giraffe

medium

Precision loss in multiple areas of MasterRouter

Summary

Precision loss due to performing division before multiplication

Vulnerability Detail

In JalaMasterRouter, several functions including _unwrapAndTransfer() calculates tokenReturnAmount:

    function _unwrapAndTransfer(
        address wrappedTokenOut,
        address to
    ) private returns (address reminderTokenAddress, uint256 reminder) {
        // address wrappedTokenOut = path[path.length - 1];
        uint256 balanceOut = IERC20(wrappedTokenOut).balanceOf(address(this));
        if (balanceOut == 0) return (reminderTokenAddress, reminder);

        uint256 tokenOutOffset = IChilizWrappedERC20(wrappedTokenOut).getDecimalsOffset();
        uint256 tokenOutReturnAmount = (balanceOut / tokenOutOffset) * tokenOutOffset;     //@audit precision loss

When balanceOut is divided by tokenOutOffset, Solidity performs integer division. If balanceOut is not a perfect multiple of tokenOutOffset, the division will result in a truncated integer, losing the fractional part.

This happens also in other functions like removeLiquidityAndUnwrapToken and removeLiquidityETHAndUnwrap.

Impact

Users suffer from the precision loss, getting out less than they expect to.

Code Snippet

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

Tool used

Manual Review

Recommendation

Amend to uint256 tokenOutReturnAmount = balanceOut * tokenOutOffset / tokenOutOffset;

nevillehuang commented 7 months ago

Invalid, duplicate of #158, but llack detailed numerical example/PoC and description of impact required by sherlock