High Risk
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L169-L175
In the current implementation of the EURO burn function within SmartVaultV3.sol, there are issues with the fee calculation and collateral requirement assessment. The issues arise when a user intends to burn a certain amount of EUROs and the associated fee, leading to potential undercollateralization risks.
Firstly, the burn fee, calculated at for example 2% of the EUROs to be burned, is currently deducted from the amount the user intends to burn, rather than being an independent transaction. This results in the user needing additional EUROs to cover the fee separately from the amount they want to burn (in case the user wants to burn all the EUROs it will revert).
Secondly, when a user burns a portion of their EUROs, the function correctly executes the burn, but fails to adjust the collateral requirement in proportion to the EUROs burned.
function burn(uint256 _amount) external ifMinted(_amount) {
uint256 fee = _amount * ISmartVaultManagerV3(manager).burnFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
minted = minted - _amount;
EUROs.burn(msg.sender, _amount);
IERC20(address(EUROs)).safeTransferFrom(msg.sender, ISmartVaultManagerV3(manager).protocol(), fee);
emit EUROsBurned(_amount, fee);
}
Example Scenario:
Consider a scenario where a user has 1 Ether as collateral and has minted 1000 EUROs. Should they decide to burn 500 EUROs:
EUROs and intends to burn 500 EUROs.EUROs (500 + 10 fee) to execute the burn operation. But as function works, fee is calculated 10 EUROs, then minted parameter becomes minted = minted - _amount: 1000 - 500 = 500.EUROs burned, and user has to pay 10 EUROs as a fee.EUROs and 1 Ether as a collateral.Now the user can adjust their collateral with minted parameter, so the user needs to have as much collateral as it intended to for 500 EUROs ( minted = 500 ), but the user has actually 490 EUROs. So the user can adjust their collateral for 490 EUROs and be liquidated.
Undercollateralization Risk: Users might face undercollateralization if the collateral is not adjusted according to the remaining EURO balance after the burn.
Liquidation: Inadequate collateral coverage for the EUROs can lead to the liquidation of the vault, causing a loss of assets for the user.
Manual review.
Revise the burn function to ensure that the fee is calculated separately from the EUROs to be burned, rather than being deducted from the user's existing EURO balance. After each burn operation, recalculate the required collateral based on the remaining EURO balance to maintain proper collateralization.
I agree that it's invalid.
If you go to the website, you can see that it displays everything a user needs to understand: Burn Fee, Actual Repayment, Send. You can check any vault, go to "Borrow" and "Repay" to see the interface https://app.thestandard.io/Collateral/150?view=2
Undercollateralization Risk: Users might face undercollateralization if the collateral is not adjusted according to the remaining
EURObalance after theburn.
I don't see how burning the debt can lead to undercollateralization. Paying back the debt can actually make undercollateralization less likely. We don't burn collateral here; we burn debt and ask the user to pay a fee in debt tokens (EUROs), not in collateral. Adjusting collateral is unrelated to the issue.
Liquidation: Inadequate collateral coverage for the
EUROs can lead to the liquidation of the vault, causing a loss of assets for the user.
While the statement is true, it's unrelated to the described issue.
Considering the points above, it should go under the category "User experience and design improvement" https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid
Minor inconveniences or design opinions with no clear loss of funds indication.
Or it should be considered low https://docs.codehawks.com/hawks-auditors/how-to-evaluate-a-finding-severity
Low Impact:
- Funds are not at risk.
- However, a function might be incorrect, state might not be handled appropriately, etc.
Agreed with the comments before. I want to add that the EUROs can still be acquired externally on Camelot. Other protocols like AAve use this mechanism too. So yes this is design.
I don't agree with the comments because none of the links provided describes the situation written here. Even if it is the design of a protocol, it is a flawed design. Users should always have collateral matching the ACTUAL balance of EUROs. In the example provided, the user has 490 euros. Why should the user need to have collateral that matches 500 EUROs? Suppose the percentage is high; the user has 400 EUROs but needs to have collateral for 500 EUROs. Why? And then, they are being liquidated because they needed to have collateral for 500 EUROs.
Additionally, if the user can externally have EUROs, they can have 1000 EUROs, but they must have collateral for 500. In this case, they should not be liquidated. It is not correct. In my opinion, the issue is a valid high issue!
The thing is that you are assuming that the vault has 1000 euros and after burning 500 euros, the balance becomes 490 euros.
NO, its not the case the amount that is 500 euros is burnt from vault, so if the balance of vault is 1000 euros then it will become 500 euros after burning the 500 euros. And 10 euros are deducted from msg.sender not from vault.
So, yes there is no undercollateralization, but if you think I am wrong here, then I request you to provide a detailed PoC with test code. Rest assured it is an invalid issue and this is how the protocol works and is totally correct.
Your understanding is correct. In the current implementation, when a user burns EUROs, the amount is deducted from the vault's balance, but the fee is deducted from the user's balance. However, it's important to note that while the user only has 490 euros actually, the vault shows 500 euros (minted), requiring the user to collateralize for 500 euros rather than 490 euros. This discrepancy arises because the project deducts the fee from outside the vault's accounting. Consequently, if a user intends to burn all their EUROs and lacks additional assets to cover the fee, they will be unable to execute the burn operation. This is because the fee is deducted from the user's balance, not from the vault's accounting. Therefore, users must retain enough EUROs to cover the fee to successfully execute the burn operation. In cases where a user wishes to burn all their EUROs but lacks additional assets to cover the fee, they cannot execute the burn operation. For example, if a user has all their assets invested in this protocol and no additional funds to cover the fee required for burning, they cannot proceed with the burn operation.
For the POC, we can just use the function burn(uint256 _amount) with specific inputs as you have already calculated in your previous response. The thing isn't solely about incorrect calculations, it's about the inconvenience for users who must cover the fee from their own assets. This setup introduces complexities. Users may find it challenging to manage their collateral effectively, especially when they must allocate additional assets just to cover fees, increasing the likelihood of undercollateralization and subsequent liquidation events.