sherlock-audit / 2024-10-axion-judging

7 stars 2 forks source link

Naresh - SolidlyV2AMO and SolidlyV3AMO: USDT Approval Logic Causes Reversion #47

Open sherlock-admin2 opened 1 month ago

sherlock-admin2 commented 1 month ago

Naresh

Medium

SolidlyV2AMO and SolidlyV3AMO: USDT Approval Logic Causes Reversion

Summary

As the documentation mentions, the contracts are designed to be compatible with any EVM chain and support USDT:

The smart contracts can potentially be implemented on any full-EVM chain

USD is a generic name for a reference stable coin paired with BOOST in the AMO ( USDC and USDT are the first natural candidates )

However, both the SolidlyV2AMO and SolidlyV3AMO contracts will not work with USDT, as they will revert during the _addLiquidity() and _unfarmBuyBurn() functions.

Root Cause

Both SolidlyV2AMO and SolidlyV3AMO use OpenZeppelin's IERC20Upgradeable interface, which expects a boolean return value when calling the approve() function. However, USDT's implementation of the approve() function does not return a boolean value, which causes the contract to revert during execution.

    /**
    * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
    * @param _spender The address which will spend the funds.
    * @param _value The amount of tokens to be spent.
    */
    function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

The functions _addLiquidity() and _unfarmBuyBurn() in both contracts expect a boolean return value, causing them to revert when interacting with USDT.

example SolidlyV3AMO::_addLiquidity and SolidlyV3AMO::_unfarmBuyBurn:

 function _addLiquidity(uint256 usdAmount, uint256 minBoostSpend, uint256 minUsdSpend, uint256 deadline)
        internal
        override
        returns (uint256 boostSpent, uint256 usdSpent, uint256 liquidity)
    {
        //....

        // Approve the transfer of BOOST and USD tokens to the pool
        IERC20Upgradeable(boost).approve(pool, boostAmount);

@>      IERC20Upgradeable(usd).approve(pool, usdAmount);

        (uint256 amount0Min, uint256 amount1Min) = sortAmounts(minBoostSpend, minUsdSpend);

        uint128 currentLiquidity = ISolidlyV3Pool(pool).liquidity();
        liquidity = (usdAmount * currentLiquidity) / IERC20Upgradeable(usd).balanceOf(pool);

        // Add liquidity to the BOOST-USD pool within the specified tick range
        (uint256 amount0, uint256 amount1) = ISolidlyV3Pool(pool).mint(
            address(this), tickLower, tickUpper, uint128(liquidity), amount0Min, amount1Min, deadline
        );

        // Revoke approval from the pool
        IERC20Upgradeable(boost).approve(pool, 0);
@>      IERC20Upgradeable(usd).approve(pool, 0);

      //....
    }

     function _unfarmBuyBurn(
        uint256 liquidity,
        uint256 minBoostRemove,
        uint256 minUsdRemove,
        uint256 minBoostAmountOut,
        uint256 deadline
    )
        internal
        override
        returns (uint256 boostRemoved, uint256 usdRemoved, uint256 usdAmountIn, uint256 boostAmountOut)
    {
       //....
        // Approve the transfer of usd tokens to the pool
@>      IERC20Upgradeable(usd).approve(pool, usdRemoved);

        // Execute the swap and store the amounts of tokens involved
        (int256 amount0, int256 amount1) = ISolidlyV3Pool(pool).swap(
            address(this),
            boost > usd, // Determines if we are swapping USD for BOOST (true) or BOOST for USD (false)
            int256(usdRemoved),
            targetSqrtPriceX96,
            minBoostAmountOut,
            deadline
        );

        // Revoke approval from the pool
@>      IERC20Upgradeable(usd).approve(pool, 0);

        //...
    }

SolidlyV2AMO::_addLiquidity and SolidlyV2AMO::_unfarmBuyBurn faces the same issue

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Adding liquidity and farming will fail due to a revert on USDT approvals

Mitigation

Use safeApprove instead of approve

sherlock-admin2 commented 2 days ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/AXION-MONEY/liquidity-amo/pull/5