fix: facade functions revert on no-ops #272
Conversation
|
@lekhovitsky the Is this behavior intended? This is the only case in the whole Other than that, the code seems good and implements the recommendations from the findings. |
|
@StErMi If |
| if (amount <= 1) return; | ||
| unchecked { | ||
| --amount; | ||
| if (amount >= 1) { |
There was a problem hiding this comment.
Wouldn't that cause a revert if the balance of the credit account is 1? What about previous gas optimisations used here?
There was a problem hiding this comment.
I guess the point is to avoid no-op withdrawals when the balance is <= 1 (one can still specify exactly amount == 1 if this 1 wei is of any value).
There was a problem hiding this comment.
That's the main reason why I do not like to have custom logics like this one.
As @lekhovitsky said, people can just specify the exact number of tokens to withdraw to avoid this revert, and definitely it's an edge case (given that you have to have just 1 wei in the balance of your CA for this token) but still...
@lekhovitsky with gas as low as 0.805 gwei like we have right now, is it really worth having this custom behavior?
There was a problem hiding this comment.
@StErMi Still don't see a single an issue with it, tbh
| @@ -707,6 +700,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait { | |||
| uint256 forbiddenTokensMask | |||
| ) internal returns (uint256, uint256) { | |||
| (address token, int96 quotaChange, uint96 minQuota) = abi.decode(callData, (address, int96, uint96)); // U:[FA-34] | |||
| if (quotaChange == 0) revert AmountCantBeZeroException(); // U:[FA-34] | |||
There was a problem hiding this comment.
@StErMi probably for completeness this line should go after quotaChange is rounded; something like
quotaChange = quotaChange != type(int96).min
? quotaChange / int96(uint96(PERCENTAGE_FACTOR)) * int96(uint96(PERCENTAGE_FACTOR))
: quotaChange;
if (quotaChange == 0) revert AmountCantBeZeroException(); // U:[FA-34]
Fixes: