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

Wrong calculation in "canRemoveCollateral" prevents user from removing the maximum allowed collateral

0xrs

Summary

Due to a logic error in the canRemoveCollateral() function, the user cannot retrieve the maximum allowed collateral that corresponds with his borrowed amount of EUROs.

Vulnerability Details

For a small borrowed amount, the user is forced to keep a fairly large amount of collateral. In the test scenario below, we can see that the collateral rate (which should be a factor of 1.2) goes up to a factor of 45.7

Let's take a simple example:

  • We provide 1 ETH of collateral

  • We borrow 100 EUROs (mint function)

  • We burn 95 EUROs (burn function)

At this stage, the minted amount = 100 EUROs - 95 EUROs + 0.5 EUROs (fee) = 5.5 EUROs
The collateral rate used in the unit tests is a factor of 1.2
This means, the minimum required collateral needs to be: 5.5 * 1.2 = 6.6 EUROs
In our test file, we use: 1ETH = 1509.43 EUROs => 6.6 EUROs = 0.00437ETH
So, the minimum collateral required in ETH is 0.00437ETH
This means, we should be able to retrieve up to: 1ETH - 0.00437ETH = 0.995ETH

The currently used logic forces the following conditions:

currentMintable >= eurValueToRemove

currentMintable is 1257.86 EUROs => this means we must leave at least 251.5 EUROs (or 0.1666ETH) as collateral and can retrieve only 1 - 0.1666 = 0.829 ETH

However, we only have a minted amount of 5.5 EUROs => so, this would be a collateral rate of: 251.5 / 5.5 = 45.7 instead of 1.2 !

The second condition is also incorrect:

minted <= currentMintable - eurValueToRemove

5.5 EUROs <= 1257.86 EUROs - eurValueToRemove => eurValueToRemove can be at most: 1257.86 EUROs - 5.5 EUROs = 1252.36 EUROs == 0.829ETH

To demonstrate the problem with the canRemoveCollateral() function, the following Test/POC can be used:

1: Add the following test to the smartVault.js file :

it('should allow borrower to remove collateral at the defined collateral rate', async () => {
//approve for fees
await EUROs.connect(user).approve(Vault.address, ethers.utils.parseEther('1'))
​
//provide collateral, borrow 100 EUROs and burn 95 EUROs afterwards
await user.sendTransaction({ to: Vault.address, value: ethers.utils.parseEther('1') }) //collat 1ETH
await Vault.connect(user).mint(user.address, ethers.utils.parseEther('100')) //borrow 100 EUROs
await Vault.connect(user).burn(ethers.utils.parseEther('95')) //burn 95 EUROs
​
//currently minted amount
const minted = (await Vault.status()).minted //currently minted amount: 100 - 95 + 0.5 (fees) = 5.5 EUROs
expect(minted).to.equal(ethers.utils.parseEther('5.5'))
​
const collateralRequiredForMinted = minted.mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC) // 6.6 EUROs
console.log(ethers.utils.formatEther(collateralRequiredForMinted))
​
//Collateral required for 5.5€ = 5.5 * 1.2 = 6.6€ in ETH
//1ETH = 1509.43€ => 6.6€ = 0.00437ETH
//The minimum collateral required is 0.00437ETH
//This means, we should be able to retrieve up to: 1ETH - 0.00437ETH = 0.995ETH
​
await expect(Vault.connect(user).removeCollateralNative(ethers.utils.parseEther('0.996'), user.address)).to.be.revertedWith(
'err-under-coll'
)
await expect(Vault.connect(user).removeCollateralNative(ethers.utils.parseEther('0.995'), user.address)).not.to.be.reverted //PROBLEM : This Fails!!!
​
//with the current canRemoveCollateral function, if we try to retrieve 0.83 ETH, the function reverts =>
//this corresponds with a collateral rate of: 45.7 :
// await expect(Vault.connect(user).removeCollateralNative(ethers.utils.parseEther('0.83'), user.address)).to.be.revertedWith(
// 'err-under-coll'
// )
​
//with the current canRemoveCollateral function we can only retrieve up to 0.82 ETH
//await expect(Vault.connect(user).removeCollateralNative(ethers.utils.parseEther('0.82'), user.address)).not.to.be.reverted
})

Impact

Depending on the amount of EUROs minted, the user may not be able to remove a sizeable portion of his collateral. Therefore, the protocol does not work as announced and users may be reluctant to use it.

Tools Used

Manual Review

Recommendations

Replace the current canRemoveCollateral() function:

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

With the following modified function:

function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
if (minted == 0) return true;
uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
uint256 euroCollateral = euroCollateral();
​
if (eurValueToRemove >= euroCollateral) return false;
​
uint256 collateralEuroLeft = euroCollateral - eurValueToRemove;
uint256 maxMintableWithCollateraleLeft = (collateralEuroLeft * ISmartVaultManagerV3(manager).HUNDRED_PC()) / ISmartVaultManagerV3(manager).collateralRate();
​
return minted <= maxMintableWithCollateraleLeft;
}
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!