Platform: Code4rena
Start Date: 09/12/2022
Pot Size: $36,500 USDC
Total HM: 9
Participants: 69
Period: 3 days
Judge: Picodes
Total Solo HM: 2
Id: 190
League: ETH
Rank: 49/69
Findings: 1
Award: $28.12
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xSmartContract
Also found by: 0Kage, 0x52, 0xAgro, 0xNazgul, 0xTraub, 0xhacksmithh, Awesome, Aymen0909, Bnke0x0, Englave, Janio, Mukund, Parth, RaymondFam, Rolezn, SmartSek, Tointer, UdarTeam, Udsen, Zarf, caventa, chaduke, csanuragjain, deliriusz, gz627, idkwhatimdoing, izhelyazkov, joestakey, neumo, obront, oyc_109, rvierdiiev, shark, trustindistrust, wait, yongskiws
28.124 USDC - $28.12
https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L47 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L67
There are some checks in Collateral contract:
Line 47
if (depositFee > 0) { require(_fee > 0, "fee = 0"); }
Line 67
if (withdrawFee > 0) { require(_fee > 0, "fee = 0"); }
That check that, if a depositFee/withdrawFee is set to a value greater than zero, the call will revert if no fee is collected.
But due to the different values of fees (1 - 100,000) and the different token decimals that can be used (based on the baseToken set in the Collateral contract), the minimum value of baseToken that a user can deposit or withdraw can differ much depending on the baseToken used.
For example if we suppose we have:
depositFee of 1,000 (0.1%)The minimum amount of tokens that can be deposited is 1,000, because FEE_DENOMINATOR = 1,000,000 and for less than 1,000 tokens, this line:
uint256 _fee = (_amount * depositFee) / FEE_DENOMINATOR;
Would result in _fee = 0.
The thing is if baseToken is WETH, 1,000 wei is a ridiculous amount, but for instance, if using the EURS token, which has only 2 decimals, the minimum deposit would be of 10€.
The same applies to withdrawals.
Manual review
Don't revert if the fee collected is 0, because it would not be profitable for anyone to make microdeposits, as the gas costs would be way higher than the fees that the user would not pay.
#0 - Picodes
2022-12-17T18:52:26Z
Why would the fact that there is an inconsistency be a security issue in itself ? The finding is interesting but of low severity in my opinion.
#1 - c4-judge
2022-12-17T18:52:33Z
Picodes changed the severity to QA (Quality Assurance)
#2 - neumoxx
2022-12-17T18:56:36Z
The thing is users could possibly be unable to withdraw their funds if they don't have the minimum withdrawable amount, and would be forced to deposit again to be able to withdraw the full amount.
#3 - c4-judge
2022-12-19T13:53:17Z
Picodes marked the issue as grade-b
#4 - ghost
2022-12-22T10:44:16Z
The thing is users could possibly be unable to withdraw their funds if they don't have the minimum withdrawable amount, and would be forced to deposit again to be able to withdraw the full amount.
This is a good point
#5 - c4-sponsor
2022-12-22T10:45:16Z
davidprepo marked the issue as sponsor acknowledged
#6 - neumoxx
2023-01-11T08:22:23Z
Hi @Picodes , just want to make sure you read my comment above about the impossibility to withdraw funds in certain scenarios, that would force users to deposit again to then withdraw (and the response by the sponsor below).
#7 - Picodes
2023-01-13T16:28:16Z
@neumoxx it is indeed a good point, but wasn't in the original impact described.
I do agree with you that it could lead to a poor UX in some cases, but I don't see how this kind of issue (a minimum amount to deposit) falls within the definition of Medium severity by C4: "Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements."
We could argue that the functionality is impacted as there is a minimum amount required to deposit but I feel it's only a matter of UX as it's easily bypassable.
#8 - neumoxx
2023-01-13T19:01:35Z
@Picodes in the original impact I put the example of EURS where the minimum deposit/withdrawal would be of 10€, so it's implicit in that statement that
...if they don't have the minimum withdrawable amount, and would be forced to deposit again to be able to withdraw the full amount
I agree with you that there's no loss of funds and that the UX can help avoid reverts, but the whole report was about not being reasonable that for EURS the minimum deposit/withdrawal would be 10€ value and for WETH it would be 0,000...0001€ (hence the title of the issue). And that could impact the experience of users and the protocol.
Anyway, I respect your decision, just wanted to explain what I wanted to report in the first place.