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

Collateral cannot be withdrawn from the Smart Vault in one transaction, and requires many transactions to pull them all out

neocrao

Summary

Smart Vault holds user's collateral at a specified collaterization rate. A user can withdraw their collateral until the minimum collaterization limit is met using SmartVaultV3::removeCollateralNative() and SmartVaultV3::removeCollateral() functions. But the checks in the contract does not allow the user to pull the collateral in one transaction, and it can require multiple transactions to pull them all out. This can mislead the user into thinking that either their collateral cannot be withdrawn, and if they somehow understand that it requires multiple transactions, it will require the user to spend a lot of gas and craft a brute force approach to actually pull all of the collateral out.

Vulnerability Details

A user can deposit collateral into SmartVaultV3, and mint EUROs against it. The amount of EUROs that can be minted will need to ensure that the collaterization is 120% of the minted value at least.

So, if a user deposits X ETH worth of collateral, then the vault will allow minting of Y EUROs token, such that:

Collaterization Rate
(X ETH Collateral Amount in EUROs) >= ----------------------- x Y EUROs token
Hundred Percent

So the user should be allow to withdraw any excess collateral based on the number of EURO's token that were minted. There are two functions that let users do this - SmartVaultV3::removeCollateralNative() and SmartVaultV3::removeCollateral().

The two functions use SmartVaultV3::canRemoveCollateral() to calculate if the vault will be undercollaterized. It does so by following the steps as outlined below:

  1. Calculate the max amount of EUROs that can be minted

  2. Calculate the EUROs equivalent amount of the collateral that the user wants to withdraw

  3. If the first amount is atleast the second amount, AS WELL AS, if the difference between the amounts is atleast the already minted amount, then its ok to withdraw the collateral

The problem is in the first condition, where it ensures that the first amoutn is atleast the second amount.

The first amount takes into account the collaterization rate and the current balances, which can come out to be less value that the EUROs amount of the collaterization that the user wants to remove. This is best explained with an example below:

Withdraw max collateral in a single attempt:

Lets say the collaterization rate is 120%
​
Lets say the user puts around 10 wei equivalent of EUROs token, then:
* Total Collateral = 10 wei
* The max mintable EUROs will be => 10/120% = ~8.33 wei
* The remaining is over-collaterization => 10 wei - 8.33 wei = 1.67 wei
​
The user can only withdraw at most ~8.33 wei :(

Withdraw max collateral in multiple attempts:

Lets say the collaterization rate is 120%
​
Lets say the user puts around 10 wei equivalent of EUROs token, then:
* Total Collateral = 10 wei
* The max mintable EUROs will be => 10/120% = ~8.33 wei
* The remaining is over-collaterization => 10 wei - 8.33 wei = 1.67 wei
​
User withdraws 8 wei.
​
New state of the vault:
* Total Collateral = 2 wei
* The max mintable EUROs will be => 2/120% = ~1.67 wei
* The remaining is over-collaterization => 2 wei - 1.67 wei = 0.33 wei
​
User withdraws 1 wei.
​
New state of the vault:
* Total Collateral = 1 wei
* The max mintable EUROs will be => 2/120% = ~0.83 wei
* The remaining is over-collaterization => 1 wei - 0.83 wei = 0.17 wei
​
The user was able to withdraw a total of => 8 wei + 1 wei = 9 wei :D

The above example demonstrate how the accounting is incorrect to ensure that minimum collaterization requirements are met when the user wants to withdraw their collateral.

Proof of Concept

Drop the below test in test/smartVault.js

(The test is using describe.only, so when you run the test, then only this test will be run)

describe.only('[PoC] Multiple transactions needed to pull ETH collateral out', async () => {
it('[PoC] Multiple transactions needed to pull ETH collateral out', async () => {
await EUROs.connect(user).approve(Vault.address, ethers.utils.parseEther('10000'));
​
const collateral = ethers.utils.parseEther('1');
await user.sendTransaction({ to: Vault.address, value: collateral });
​
// Mint 1 EUROs token
const mintedValue = BigNumber.from(1); // 1 wei
await Vault.connect(user).mint(user.address, mintedValue);
​
// Attempt to remove (1 ETH - 1 wei) of collateral in one shot
const collateralToRemove = ethers.utils.parseEther('1').sub(BigNumber.from('1'));
let removeCollateralNativeAttemptInOneShot =
Vault.connect(user).removeCollateralNative(collateralToRemove, user.address);
// The attempt fails, as it claims that the vault will get under collaterized
await expect(removeCollateralNativeAttemptInOneShot).to.be.revertedWith('err-under-coll');
​
// Ok, lets see how much collateral can we remove by brute forcing then
let collateralRemovedSoFar = BigNumber.from("0");
// Start with the max we can remove, that is the collateral we deposited - 1 ETH
let amountToRemoveAtOneTime = ethers.utils.parseEther('1');
let countOfSuccessfulAttempts = 0;
let countOfFailedAttempted = 0;
while (true) {
try {
await Vault.connect(user).removeCollateralNative(amountToRemoveAtOneTime, user.address);
collateralRemovedSoFar = collateralRemovedSoFar.add(amountToRemoveAtOneTime);
countOfSuccessfulAttempts++;
} catch (e) {
countOfFailedAttempted++;
// If the transaction reverts, then reduce the amount we are trying to pull
// by dividing the amount by 10
// and continue the brute force
amountToRemoveAtOneTime = amountToRemoveAtOneTime.div(10);
if (amountToRemoveAtOneTime == 0) {
break;
}
}
}
​
// Well, surprisingly we were able to pull the same amount
// of collateral that we were trying to pull initially
// but we just had to brute force with smaller amounts
// expect(collateralRemovedSoFar).to.equal(collateralToRemove);
expect(collateralRemovedSoFar).to.be.at.least(collateralToRemove);
// With this brute force attempt it took 162 successful attempts
// and 19 failed attempts to pull all the funds
expect(countOfSuccessfulAttempts).to.be.at.least(162);
expect(countOfFailedAttempted).to.be.at.least(19);
});
});

Impact

There are varying impact of this issue based on the user:

  • A user might attempt to remove their collateral once, and the vault might just return part of their collateral. The uninformed user might not realize that they will need to execute multiple transactions to get their collateral

  • A user will need to spend a lot of gas, and creative brute forcing to get all of their collateral out

Tools Used

Manual Review

Recommendations

The logic that verified that the vault will not be undercollaterized upon removal of collateral will need revision. This might also impact other functions of the contract, and I will encourage the protocol to test it thoroughly as this is one of the core logic that will be updated. Below is a suggestion on the alternative approach:

When the user requests for removal of the collateral, then evaluate if the final state of the vault meets the collaterization requirement.

Below is how the code can look like for checking ETH collateral. This will need to be further adapted to work with all the supported ERC20 tokens.

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;
+ uint256 finalTokenBalanceAfterRemoval = getAssetBalance(_token.symbol, _token.addr) - _amount;
+ uint256 finalTokenBalanceAfterRemovalInEur = calculator.tokenToEurAvg(_token, finalTokenBalanceAfterRemoval);
+ uint256 maxMintableAmount = (finalTokenBalanceAfterRemovalInEur *
+ ISmartVaultManagerV3(manager).HUNDRED_PC()) /
+ ISmartVaultManagerV3(manager).collateralRate();
+
+ return maxMintableAmount >= minted;
}
Updates

Lead Judging Commences

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

canRemoveCollateral

Support

FAQs

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