c4-bot-6 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/libraries/logic/VaultLogic.sol#L627

Vulnerability details

When isolateLiquidate(supplyAsCollateral=false) is executed Finally erc721DecreaseIsolateSupplyOnLiquidate() will be executed and the NFT will be transferred to the user

  function erc721DecreaseIsolateSupplyOnLiquidate(
    DataTypes.AssetData storage assetData,
    uint256[] memory tokenIds
  ) internal {
    for (uint256 i = 0; i < tokenIds.length; i++) {
      DataTypes.ERC721TokenData storage tokenData = assetData.erc721TokenData[tokenIds[i]];
      require(tokenData.supplyMode == Constants.SUPPLY_MODE_ISOLATE, Errors.INVALID_SUPPLY_MODE);

      assetData.userScaledIsolateSupply[tokenData.owner] -= 1;

      tokenData.owner = address(0);
      tokenData.supplyMode = 0;
@>  //missing tokenData.lockerAddr = address(0);
    }

    assetData.totalScaledIsolateSupply -= tokenIds.length;
  }

We know from the above code that this method does not clear the tokenData.lockerAddr So, now tokenData is. erc721TokenData[NFT_1].owner = 0 erc721TokenData[NFT_1].supplyMode = 0 erc721TokenData[NFT_1].lockerAddr = address(poolManager)

And user alice has NFT_1, then alice execute BVault.depositERC721(NFT_1, supplyMode = SUPPLY_MODE_CROSS) will succeed, deposit() does not check lockerAddr.

So tokenData becomes. erc721TokenData[NFT_1].owner = alice erc721TokenData[NFT_1].supplyMode = SUPPLY_MODE_CROSS erc721TokenData[NFT_1].lockerAddr = address(poolManager) -> not change

After that the user's NFT_ 1 will be locked because withdrawERC721() -> validateWithdrawERC721() will check that lockerAddr must be address(0)

  function validateWithdrawERC721(
...
    for (uint256 i = 0; i < inputParams.tokenIds.length; i++) {
      DataTypes.ERC721TokenData storage tokenData = VaultLogic.erc721GetTokenData(assetData, inputParams.tokenIds[i]);
      require(tokenData.owner == inputParams.onBehalf, Errors.INVALID_CALLER);
      require(tokenData.supplyMode == inputParams.supplyMode, Errors.INVALID_SUPPLY_MODE);

@>    require(tokenData.lockerAddr == address(0), Errors.ASSET_ALREADY_LOCKED_IN_USE);
    }
  }

Other Isolate methods cannot be operated either

Note: erc721DecreaseIsolateSupply() similar

Impact

Unable to retrieve NFT

Recommended Mitigation

  function erc721DecreaseIsolateSupplyOnLiquidate(
    DataTypes.AssetData storage assetData,
    uint256[] memory tokenIds
  ) internal {
    for (uint256 i = 0; i < tokenIds.length; i++) {
      DataTypes.ERC721TokenData storage tokenData = assetData.erc721TokenData[tokenIds[i]];
      require(tokenData.supplyMode == Constants.SUPPLY_MODE_ISOLATE, Errors.INVALID_SUPPLY_MODE);

      assetData.userScaledIsolateSupply[tokenData.owner] -= 1;

      tokenData.owner = address(0);
      tokenData.supplyMode = 0;
+     tokenData.lockerAddr = address(0);
    }

    assetData.totalScaledIsolateSupply -= tokenIds.length;
  }

  function erc721DecreaseIsolateSupply(
    DataTypes.AssetData storage assetData,
    address user,
    uint256[] memory tokenIds
  ) internal {
    for (uint256 i = 0; i < tokenIds.length; i++) {
      DataTypes.ERC721TokenData storage tokenData = assetData.erc721TokenData[tokenIds[i]];
      require(tokenData.supplyMode == Constants.SUPPLY_MODE_ISOLATE, Errors.INVALID_SUPPLY_MODE);

      tokenData.owner = address(0);
      tokenData.supplyMode = 0;
+     tokenData.lockerAddr = address(0);
    }

    assetData.totalScaledIsolateSupply -= tokenIds.length;
    assetData.userScaledIsolateSupply[user] -= tokenIds.length;
  }