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.
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:
CR defined by the protocol = Cr
Collateral value in euro that the borrower deposited to the vault = Cv
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.
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.
Manual review
Change the return statement in SmartVaultV3::canRemoveCollateral
to:
Note that the first condition in the original code, currentMintable >= eurValueToRemove
, is removed since it's unnecessary as described in the vulnerability details section.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.