code-423n4 / 2022-01-elasticswap-findings

1 stars 0 forks source link

Cache state variables #133

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

Jujic

Vulnerability details

Impact

State variables baseToken and quoteToken addresses are read several times respectively in addLiquidity(). Caching them in local variables at the beginning of the function and using those local variables can save gas.

The same situation is in the removeLiquidity()

Proof of Concept

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/Exchange.sol#L87-L156

function addLiquidity(
        uint256 _baseTokenQtyDesired,
        uint256 _quoteTokenQtyDesired,
        uint256 _baseTokenQtyMin,
        uint256 _quoteTokenQtyMin,
        address _liquidityTokenRecipient,
        uint256 _expirationTimestamp
    ) external nonReentrant() {
        isNotExpired(_expirationTimestamp);

        MathLib.TokenQtys memory tokenQtys =
            MathLib.calculateAddLiquidityQuantities(
                _baseTokenQtyDesired,
                _quoteTokenQtyDesired,
                _baseTokenQtyMin,
                _quoteTokenQtyMin,
                IERC20(baseToken).balanceOf(address(this)),
                IERC20(quoteToken).balanceOf(address(this)),
                this.totalSupply(),
                internalBalances
            );

        internalBalances.kLast =
            internalBalances.baseTokenReserveQty *
            internalBalances.quoteTokenReserveQty;

        if (tokenQtys.liquidityTokenFeeQty > 0) {
            // mint liquidity tokens to fee address for k growth.
            _mint(
                IExchangeFactory(exchangeFactoryAddress).feeAddress(),
                tokenQtys.liquidityTokenFeeQty
            );
        }
        _mint(_liquidityTokenRecipient, tokenQtys.liquidityTokenQty); // mint liquidity tokens to recipient

        if (tokenQtys.baseTokenQty != 0) {
            bool isExchangeEmpty =
                IERC20(baseToken).balanceOf(address(this)) == 0;

            // transfer base tokens to Exchange
            IERC20(baseToken).safeTransferFrom(
                msg.sender,
                address(this),
                tokenQtys.baseTokenQty
            );

            if (isExchangeEmpty) {
                require(
                    IERC20(baseToken).balanceOf(address(this)) ==
                        tokenQtys.baseTokenQty,
                    "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED"
                );
            }
        }

        if (tokenQtys.quoteTokenQty != 0) {
            // transfer quote tokens to Exchange
            IERC20(quoteToken).safeTransferFrom(
                msg.sender,
                address(this),
                tokenQtys.quoteTokenQty
            );
        }

Tools Used

Remux

Recommended Mitigation Steps

Cache baseToken and quoteToken state variables in local variables at the beginning of the function

GalloDaSballo commented 2 years ago

Disagree with the finding. quoteToken and baseToken are immutable, they get inlined in the bytecode at deploy time. This means that they are effectively already being cached, no storage read cost is ever applied to using them.

The advice would be correct if the variables weren't immutable, but because they are, am marking as invaldi