Yashar - De-Whitelisted tokens remain usable as collateral due to incomplete asset revocation

[sherlock-admin4]

Yashar

Medium

De-Whitelisted tokens remain usable as collateral due to incomplete asset revocation

Summary

The issue occurs because the protocol does not revoke or restrict the use of de-whitelisted tokens already in user positions. Even after an asset is de-whitelisted, users can still use it as collateral by transferring the token directly to their positions, circumventing the protocol's checks. This flaw allows users to exploit outdated or undesirable assets, potentially leading to inaccurate collateral assessments

Vulnerability Detail

In PositionManager.sol, every asset that a user wants to interact with should be whitelisted in the isKnownAsset mapping. For example, while making a deposit:

        // mitigate unknown assets being locked in positions
        if (!isKnownAsset[asset]) revert PositionManager_DepositUnknownAsset(asset);

The admin of the protocol has the ability to de-whitelist an asset using toggleKnownAsset:

    function toggleKnownAsset(address asset) external onlyOwner {
        isKnownAsset[asset] = !isKnownAsset[asset];
        emit ToggleKnownAsset(asset, isKnownAsset[asset]);
    }

However, if the admin adds an asset to isKnownAsset and users subsequently add that token to their positions, even if the admin removes this asset from isKnownAsset, users can still use this non-whitelisted token as collateral by directly transferring the asset to their position contract instead of calling PositionManager.sol::deposit().

PoC

• Admin adds XYZ token to isKnownAsset
• User calls addToken and adds XYZ token to their positions token list
• After some time, for any reasons, admin decides to de-whitelist the XYZ token and calls toggleKnownAsset(XYZ)
• Since the user hasn't removed the token from their position's token list, they can still make deposits by transferring assets to their position contract and using that token as collateral.

Coded PoC

Please add the following test code to LiquidationTest.t.sol:

    function test_borrowWithUnKnownAsset() public {
        vm.label(address(asset2), "asset2");
        vm.startPrank(user);
        asset2.approve(address(positionManager), 2e18);
        asset1.mint(liquidator, 10e18);

        // user opens a position and add asset2 
        Action[] memory actions = new Action[](2);
        (position, actions[0]) = newPosition(user, bytes32(uint256(0x123456789)));
        actions[1] = addToken(address(asset2));
        positionManager.processBatch(position, actions);
        vm.stopPrank();

        // protocol owner removes asset2 from isKnownAsset
        vm.prank(protocolOwner);
        positionManager.toggleKnownAsset(address(asset2));

        // if user deposit his txn will revert with DepositUnknownAsset error
        Action memory depositAction = deposit(address(asset2), 2e18);
        vm.startPrank(user);
        vm.expectRevert(
            abi.encodeWithSelector(PositionManager.PositionManager_DepositUnknownAsset.selector, address(asset2))
        );
        PositionManager(positionManager).process(position, depositAction);
        vm.stopPrank();

        Action memory borrowAction = borrow(fixedRatePool, 1e18);

        // user's borrow txn will revert because has no collateral
        vm.prank(user);
        vm.expectRevert(
            abi.encodeWithSelector(PositionManager.PositionManager_HealthCheckFailed.selector, position)
        );
        PositionManager(positionManager).process(position, borrowAction);

        // user transfers some asset2 directly to the position even when the asset2 is not in isKnownAssets
        vm.prank(user);
        asset2.transfer(address(position), 2e18);

        // user borrows money from the protocol with non-whitelisted asset as collateral
        vm.prank(user);
        PositionManager(positionManager).process(position, borrowAction);
    }

Run the test:

forge test --mt test_borrowWithUnKnownAsset

Impact

This bug has multiple impacts:

• Users are able to interact with the protocol using assets that the protocol does not want to interact with.
• The removed asset might have a stale price, leading to incorrect collateral calculations.
• The removed asset might have oracle problems, causing reverts upon price checking, which could lead to a DoS while liquidation if the position becomes unhealthy.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/PositionManager.sol#L522-L524

Tool used

• VSCode
• Foundry

Recommendation

Don't count invalid assets while checking the health of a position:

--- RiskModule.sol.orig	2024-08-24 06:30:49.124503413 +0330
+++ RiskModule.sol	2024-08-24 06:34:05.262792970 +0330
@@ -13,6 +13,7 @@
 import { RiskEngine } from "./RiskEngine.sol";
 import { IOracle } from "./interfaces/IOracle.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { PositionManager } from "./PositionManager.sol";
 
 // libraries
 import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
@@ -229,7 +230,13 @@
         uint256 positionAssetsLength = positionAssets.length;
         uint256[] memory positionAssetData = new uint256[](positionAssetsLength);
 
+        PositionManager positionManager = PositionManager(Position(payable(position)).POSITION_MANAGER());
+
         for (uint256 i; i < positionAssetsLength; ++i) {
+
+            // If the asset is not known, then don't count it as valid asset of a position
+            if ( !positionManager.isKnownAsset(positionAssets[i]) ) continue;
+
             uint256 assets = getAssetValue(position, positionAssets[i]);
             // positionAssetData[i] stores value of positionAssets[i] in eth
             positionAssetData[i] = assets;

Duplicate of #282