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

Incorrect implementation of `SmartVaultV3::canRemoveCollateral` prevents user from withdrawing the collateral even if the vault will remain fully collateralized after withdrawal

shikhar229169

Summary

In the SmartVaultV3 contract the condition for fully collateralized vault is that the current minted amount should be less than or equal to the max mintable amount on the basis of the collateral deposited by user.
But due to incorrect implementation of SmartVaultV3::canRemoveCollateral function this will affect the removal of collateral and will prevent user from withdrawing their collateral even if the vault remains fully collateralized after removal of collateral, thus affecting the functioning of removeCollateralNative, removeCollateral and removeAsset.

Vulnerability Details

The vulnerability arose due to the incorrect implementation of SmartVaultV3::canRemoveCollateral function which returns false even if the vault will remain fully collateralized after removal of the desired collateral by user.

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;
}

Here the implementation for deciding whether the collateral can be removed or not is incorrect.
It checks that the minted amount should be less than (currentMintable - eurValueToRemove).
It checks by subtracting eurValueToRemove from currentMintable, where currentMintable is the max amount of euros a user can mint.
But it is irrelevant to subtract the eurValueToRemove from max euros a user can mint as it is not their euro collateral balance.

The correct implementation should be that, the minted amount to be less than (euroCollateral - euroValueToRemove) * HUNDRED_PC / COLLATERAL_RATE

Impact

  • User can't remove their collateral even if the vault is fully collateralized as the eurValueToRemove is subtracted from maxMintable but their actual balance is euroCollateral.

  • Thus leading to locking of their collateral tokens inside SmartVaultV3

PoC

Add the test in the file: test/smartVaultManager.js

Run the test:

yarn hardhat test test/smartVaultManager.js --grep "User can't remove their collateral even if after removing it the vault is fully collateralized"
describe("Vault Opening, Depositing Collateral, Minting EUROs, Withdrawing Collateral", () => {
it("User can't remove their collateral even if after removing it the vault is fully collateralized", async () => {
// collateral rate 120%
// means for minting 100 EUROs token, collateral should be equivalent to atleast 120 euros
// euro to usd - $1.06
​
// mint fee rate - 0.5%
// collateral - eth
// eth to usd - $1600
​
// lets deposit 120 euros eq of eth
// eth collateral required = (120 * 1.06) / 1600 = 0.0795
​
const ethCollateralAmount = ethers.utils.parseEther("0.0795")
// first open a vault
await VaultManager.connect(user).mint()
let tokenId, status
let vaultAddress
​
({ tokenId, status } = (await VaultManager.connect(user).vaults())[0]);
({ vaultAddress } = status);
const vault = await ethers.getContractAt("SmartVaultV3", vaultAddress)
​
​
// for 20 euros mint
// fee on 20 euros = 0.5% of 20 = 0.1 euros
// therefore total collateral req for 20.1 euros = 24.12
// total eth req = (24.12 * 1.06) / 1600 = 0.0159795
await user.sendTransaction({ to: vaultAddress, value: ethCollateralAmount })
const eurosMintAmount = ethers.utils.parseEther("20")
await vault.connect(user).mint(user.address, eurosMintAmount)
​
// now the total minted amount = 20.1 euros (0.1 is minted for fee rates)
// as the total eth collateral req for fully collateralization is 0.0159795,
// the user should be able to withdraw the remaining eth
// i.e., (0.0795 - 0.0159795) = 0.0635205
​
// the user should be able to remove this collateral eth amount
// but due to implementation issue for collateral checks, user can't remove their collateral
// even though the vault is fully collateralized after removing that amount of collateral
const removeCollateralEthAmount = ethers.utils.parseEther("0.0635205")
await expect(vault.connect(user).removeCollateralNative(removeCollateralEthAmount, user.address)).to.be.revertedWith("err-under-coll")
})
})

Tools Used

Manual Review

Recommendations

Correct the implementation of canRemoveCollateral as below:

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);
+ uint256 remainingCollateral = euroCollateral() - eurValueToRemove;
+ uint256 mintableAfterRemovingCollateral = remainingCollateral * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
+ return minted <= mintableAfterRemovingCollateral;
- return currentMintable >= eurValueToRemove &&
- minted <= currentMintable - eurValueToRemove;
}
Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

canRemoveCollateral

Support

FAQs

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

Give us feedback!