The Standard

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

Wrong calculation in `SmartVaultV3::canRemoveCollateral`, borrowers can't remove all of their collateral down to the defined collateral ratio.

Summary

Borrowers who want to remove their own collateral via SmartVaultV3::removeCollateralNative or SmartVaultV3::removeCollateral or SmartVaultV3::removeAsset will not be able to remove it down to the defined collateral ratio (CR) of the protocol, effectively forcing them to a lower CR than the protocol defined.

Vulnerability Details

For borrowers to be allowed to borrow (mint) EUROs stablecoins, they need to provide collateral to the vault. The amount of collateral they provide, determines the amount of EUROs they are allowed to borrow. This relationship is what is known as the collateral ratio, and mathematically it means max_EUROs_allowed_to_borrow=collateral/CR. So for example, if the CR is defined to be equal to 120%=1.2, then for a 120 euro worth of collateral, the borrower can borrow at maximum 120/1.2=100 EUROs.

Of course it does not mean that borrowers must maximize their borrowing power by borrowing the maximum amount. They can borrow less than the maximum (or even not borrow at all even though they have collateral deposited), which effectively makes their current CR much higher. Continuing the example above, the borrower only borrowed (minted) 60 EUROs, so their current CR = 120/60=2=200%. In such a case, the borrower is allowed to remove some of his collateral down to the point where the current CR matches the protocol’s defined CR, which in the example above is 1.2.

To check if and how much a borrower can remove from his collateral, the private view function SmartVaultV3::canRemoveCollateral is called by the exposed functions for removing collateral, SmartVaultV3::removeCollateralNative, SmartVaultV3::removeCollateral and SmartVaultV3::removeAsset.

This view function checks the current amount already minted by the borrower and how much collateral the borrower holds to determine if collateral can be removed. The problem arises in the return statement seen here https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L131. This statement gets the mathematical calculation wrong and effectively forces the borrower to CR=100%=1 while the protocol can define any value for CR (but according to test it is 120%).

To see how this is a wrong calculation, consider the following mathematical treatment:

  1. CR defined by the protocol = Cr

  2. Collateral value in euro that the borrower deposited to the vault = Cv

  3. Maximum amount that can be borrowed = Mmax = Cv/Cr

Assume borrower already minted some EUROs. Define this amount as M, where M<Mmax and M≠0 because this is already treated here https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L128.

Let’s calculate how much collateral (in euro) is needed to not be under-collateralized. Define this value as Cv_needed, and we get: Cv_needed = M * Cr.

This means that we can remove up to Cv-Cv_needed. if we define this as Cremove we get:

Cremove ≤ Cv-Cv_needed = Mmax*Cr - M*Cr = Cr*(Mmax-M).

So from this treatment we see that we should check for Cr*(Mmax - M) ≥ Cremove.

The code checks for minted <= currentMintable - eurValueToRemove, or if we rearrange currentMintable - minted >= eurValueToRemove, where currentMintable = Mmax, minted = M and eurValueToRemove = Cremove.

Compare Cr*(Mmax - M) ≥ Cremove to (Mmax - M) ≥ Cremove, and we can see as if the code hardcoded CR=1 meaning a 100% collateral ratio.

This means that the borrower cannot recover a portion of his/her collateral. Let’s calculate exactly how much:

The borrower should recover Cr*(Mmax - M) ≥ Cremove but in practice they can recover only (Mmax - M) ≥ Cremove so they essentially lose the difference between the two. If we divide by the amount that they should have recovered, we can see how much percentage loss they incur:

(Cr*(Mmax - M) - (Mmax - M))/(Cr*(Mmax - M)) = …some steps… = 1 - 1/Cr. For Cr=1.2 this means the borrower cannot recover 16.7% (or sixth) of their collateral. This collateral is stuck in the vault forever.

A POC is an actual example with numbers:

Cr = 1.2, Cv = 1200 euro, we get Mmax = 1200 / 1.2 = 1000 euro. Assume we minted 600 EUROs, we should be able to remove 1.2*(1000-600)=480 euro worth of collateral. But the code allows us only to recover 1000-600=400.

Note another issue with the return statement in the code: the first condition, currentMintable >= eurValueToRemove is not doing anything actually. Compare currentMintable >= eurValueToRemove to currentMintable - minted >= eurValueToRemove (the second condition). When the second condition is TRUE, then obviously the first one is TRUE too because currentMintable - minted will always be smaller than currentMintable, and if the the second one is FALSE then it doesn't matter what the first one returns.

Impact

Borrowers who are collateralized more than the protocol’s defined CR and already minted EUROs, won’t succeed to regain some of their native or ERC20 token collateral down to the defined CR, causing the collateral to be stuck in their vault.

Tools Used

Manual review

Recommendations

Change the return statement in SmartVaultV3::canRemoveCollateral to:

- return currentMintable >= eurValueToRemove && minted <= currentMintable - eurValueToRemove;
+ return minted <= currentMintable - (eurValueToRemove * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate());

Note that the first condition in the original code, currentMintable >= eurValueToRemove, is removed since it's unnecessary as described in the vulnerability details section.

Updates

Lead Judging Commences

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

canRemoveCollateral

aamirusmani1552 Auditor
over 1 year ago
stakog Submitter
over 1 year ago
kose Auditor
over 1 year ago
aamirusmani1552 Auditor
over 1 year ago
stakog Submitter
over 1 year ago
aamirusmani1552 Auditor
over 1 year ago
stakog Submitter
over 1 year ago
aamirusmani1552 Auditor
over 1 year ago
hrishibhat Lead Judge
over 1 year ago
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.