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

6 stars 4 forks source link

0x77 - "Stack too deep" error in JalaMasterRouter #216

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

0x77

high

"Stack too deep" error in JalaMasterRouter

High

Summary

The contract JalaMasterRouter.sol has compilation error,

Vulnerability Detail

The error exists in the function "wrapTokensAndaddLiquidity" in JalaMasterRouter.sol making it unable to deploy. The error is due to use of more than allowed local variables i.e 16.

Impact

Tha JalaMasterRouter.sol contains a lot of protocol's important function , unable to deploy it can disturb the whole protocol working.

Code Snippet

 function wrapTokensAndaddLiquidity(
        // only use wrapTokensAndaddLiquidity to create pool.
        address tokenA, // origin token
        address tokenB, // origin token
        uint256 amountADesired, // unwrapped.
        uint256 amountBDesired, // unwrapped.
        uint256 amountAMin, // unwrapped
        uint256 amountBMin, // unwrapped
        address to,
        uint256 deadline
    ) public virtual override returns (uint256 amountA, uint256 amountB, uint256 liquidity) {
        // get token from user
        TransferHelper.safeTransferFrom(tokenA, msg.sender, address(this), amountADesired);
        TransferHelper.safeTransferFrom(tokenB, msg.sender, address(this), amountBDesired);

        address wrappedTokenA = _approveAndWrap(tokenA, amountADesired);
        address wrappedTokenB = _approveAndWrap(tokenB, amountBDesired);

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

        IERC20(wrappedTokenA).approve(router, IERC20(wrappedTokenA).balanceOf(address(this))); // no need for check return value, bc addliquidity will revert if approve was declined.
        IERC20(wrappedTokenB).approve(router, IERC20(wrappedTokenB).balanceOf(address(this)));

        // add liquidity
        (amountA, amountB, liquidity) = IJalaRouter02(router).addLiquidity(
            wrappedTokenA,
            wrappedTokenB,
            amountADesired * tokenAOffset,
            amountBDesired * tokenBOffset,
            amountAMin * tokenAOffset,
            amountBMin * tokenBOffset,
            to,
            deadline
        );
        _unwrapAndTransfer(wrappedTokenA, to);
        _unwrapAndTransfer(wrappedTokenB, to);
    }

https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaMasterRouter.sol?plain=1#L32

Tool used

Manual Review Remix IDE

Recommendation

in order to mitigate this issue, we can reduce the local variables. i have omitted "tokenAOffset " and "tokenBOffset "

 function wrapTokensAndaddLiquidity(
        // only use wrapTokensAndaddLiquidity to create pool.
        address tokenA, // origin token
        address tokenB, // origin token
        uint256 amountADesired, // unwrapped.
        uint256 amountBDesired, // unwrapped.
        uint256 amountAMin, // unwrapped
        uint256 amountBMin, // unwrapped
        address to,
        uint256 deadline
    )
        public
        virtual
        override
        returns (
            uint256 amountA,
            uint256 amountB,
            uint256 liquidity
        )
    {
        // get token from user
        TransferHelper.safeTransferFrom(
            tokenA,
            msg.sender,
            address(this),
            amountADesired
        );
        TransferHelper.safeTransferFrom(
            tokenB,
            msg.sender,
            address(this),
            amountBDesired
        );

        address wrappedTokenA = _approveAndWrap(tokenA, amountADesired);
        address wrappedTokenB = _approveAndWrap(tokenB, amountBDesired);
        IERC20(wrappedTokenA).approve(
            router,
            IERC20(wrappedTokenA).balanceOf(address(this))
        ); // no need for check return value, bc addliquidity will revert if approve was declined.
        IERC20(wrappedTokenB).approve(
            router,
            IERC20(wrappedTokenB).balanceOf(address(this))
        );

        // add liquidity
        (amountA, amountB, liquidity) = IJalaRouter02(router).addLiquidity(
            wrappedTokenA,
            wrappedTokenB,
            amountADesired *
                IChilizWrappedERC20(wrappedTokenA).getDecimalsOffset(),
            amountBDesired *
                IChilizWrappedERC20(wrappedTokenB).getDecimalsOffset(),
            amountAMin * IChilizWrappedERC20(wrappedTokenA).getDecimalsOffset(),
            amountBMin * IChilizWrappedERC20(wrappedTokenB).getDecimalsOffset(),
            to,
            deadline
        );
        _unwrapAndTransfer(wrappedTokenA, to);
        _unwrapAndTransfer(wrappedTokenB, to);
    }
nevillehuang commented 6 months ago

Invalid, no issues here. There are existing tests in place as seen here verifying compilation is possible.