Platform: Code4rena
Start Date: 14/07/2022
Pot Size: $25,000 USDC
Total HM: 2
Participants: 63
Period: 3 days
Judge: PierrickGT
Total Solo HM: 1
Id: 147
League: ETH
Rank: 4/63
Findings: 2
Award: $401.22
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: hickuphh3
Also found by: 0x29A, 0x52, 0xNazgul, Chom, Deivitto, ElKu, Funen, IllIllI, Meera, ReyAdmirado, SooYa, TomJ, Trumpero, Waze, __141345__, ak1, asutorufos, c3phas, cRat1st0s, csanuragjain, delfin454000, exd0tpy, fatherOfBlocks, hake, hansfriese, horsefacts, hyh, karanctf, kenzo, kyteg, ladboy233, pashov, peritoflores, rajatbeladiya, rbserver, reassor, rokinot, simon135, wastewa
381.8352 USDC - $381.84
The revised witch (liquidation engine) contract includes the following improvements over the previous version. As stated in the README, they are:
fyToken.The liquidations flow was quite easy to follow as it consists of the following:
auction()payBase() and payFYToken()cancel() is called to prematurely end the auctionThe README was very extensive and thorough, and succinctly explained design considerations made. Flow diagrams were provided to help visualise the interactions required between different contracts. Inline comments were appropriate too, aided in understanding the functionality.
All foundry tests passed as expected. One area of improvement is to have mainnet forking tests, since mocking is used for the external contracts. Running forge coverage unfortunately didn’t work. I suspect it is due to the instability of the feature rather than the fault of the tests.
Casting could be avoided if input / output params were defined appropriately. For instance, inkOut, artIn in _updateAccounting(), and liquidatorCut and auctioneerCut could have been uint128 instead of uint256.
If a vault becomes over-collateralised after a partial liquidation, it is still subject to further liquidation as the auction isn’t closed. The vault owner has to call cancel() himself, or trust other altruistic actors to perform this action on his behalf. Liquidators will unlikely do it because they are economically incentivised not to do so.
One can however argue that this is mitigated by the fact that protocol (governance) sets the vault proportion that can be auctioned. Regardless of whether the fact that the vault is over-collateralised after partial liquidations, the liquidators arguably are given the right to carry out further liquidations up to the proportion set.
Nevertheless, a reason for a revised liquidations witch contract is that “More often than not, liquidated users have lost all their collateral as we have failed to make liquidations competitive.”. Hence, it might make sense to ensure that users need not lose more collateral than necessary.
Consider checking if the vault is over-collateralized (maybe in _updateAccounting()) and close the auction if it is. This however adds complexity to the liquidation logic, as you have to update the cauldron first cauldron.slurp() before checking and updating the collateralization status. It will also break the CEI pattern, which might be unfavourable.
_calcPayout() should include equality- else if (elapsed > duration) + else if (elapsed >= duration)
In the case where elapsed == duration, proportionNow evaluates to 1e18, which is the same result when elapsed > duration. Proof below.
proportionNow = uint256(initialProportion) + uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration)); // = initialProportion + (1e18 - initialProportion).wmul(1e18) // = initialProportion + (1e18 - initialProportion) * 1e18 / 1e18 // = initialProportion + 1e18 - initialProportion // = 1e18
Of slightly greater importance, this handles the edge case when elapsed = duration = 0, ie. the liquidation transaction is included in the same block / has the same timestamp as the auction initialization transaction
As per the TLDR.
Since the proportion given for zero duration auctions is 1e18, it is equivalent to an auction of infinite duration with 100% initial offer: duration == type(uint32).max and line_.initialOffer = 1e18.
auctioneerCutTechnically, the auctioneerCut goes to the to address specified by the auctioneer when auction() is called, which, while unlikely, may not be the auctioneer himself. Also, the comparison is done against the to address specified, not the caller / msg.sender as the comment implies.
- Amount paid to whomever started the auction. 0 if it's the same address that's calling this method + Amount paid to address specified by whomever started the auction. 0 if it's the same as the `to` address
setLimit()The comments seem outdated as the only parameter that is updated by the function is the maximum collateral that can be concurrently auctioned off.
/// - the auction duration to calculate liquidation prices /// - the proportion of the collateral that will be sold at auction start /// - the maximum collateral that can be auctioned at the same time /// - the minimum collateral that must be left when buying, unless buying all /// - The decimals for maximum and minimum
Suggest removing / updating the referenced comments.
The limit check is done before the summation to the total collateral allowable for liquidation. One may consider this to be a bug, but the README explains why this is the case:
Note that the first auction to reach the limit is allowed to pass it, so that there is never the situation where a vault would be too big to ever be auctioned.
The inline comments have this as well, but isn’t as clearly put as the README.
// There is a limit on how much collateral can be concurrently put at auction, but it is a soft limit. // If the limit has been surpassed, no more vaults of that collateral can be put for auction. // This avoids the scenario where some vaults might be too large to be auctioned.
For greater clarity, I would suggesting modifying the inline comment to be worded similar as the README.
// There is a limit on how much collateral can be concurrently put at auction, but it is a soft limit. - // If the limit has been surpassed, no more vaults of that collateral can be put for auction. + // The first auction to reach or exceed the limit is allowed to pass it, but subsequently, no more vaults of that collateral can be put for auction. // This avoids the scenario where some vaults might be too large to be auctioned.
- bellow + below - differente + different // Extra spacing - The Join then dishes out + The Join then dishes out - quoutes hoy much ink + quotes how much ink
#0 - alcueca
2022-07-22T14:00:41Z
This is a great QA report :heart:
#1 - PierrickGT
2022-08-12T16:48:54Z
Best QA report of this contest with clear description of the problems and comprehensive suggestions provided.
L01: Vaults that are over-collateralised after partial liquidation are possibly subject to further liquidations
Great suggestion and explanation of the risks induced by the smart contract architecture and design of the Witch contract. As mentioned by the warden, his suggestion may complexify the code but he did a good job of outlining the risks and the sponsor can now decide to implement or not his suggestion.
L02: Comparison in _calcPayout() should include equality
Great find and recommandation that will cover the edge case when elapsed = duration = 0.
L03: Incorrect description for auctioneerCut
The comment was indeed not entirely clear and the suggestion provided by the warden should avoid any confusion in the future.
L04: Incorrect natspec for setLimit()
Good recommandation, it's always best to write up to date Natspect documentations to avoid any confusion during a future refactor of the code.
NC01: Modify comment to soft limit check for clarity
Not critical but the suggestion from the warden adds a bit of clarity to the comment.
NC02: Typos
It's always best to avoid typos in the documentation of the code.
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, Aymen0909, Chom, Deivitto, ElKu, JC, JohnSmith, Kaiziron, Limbooo, MadWookie, Meera, ReyAdmirado, Rohan16, Sm4rty, SooYa, TomJ, Trumpero, Waze, __141345__, ajtra, ak1, antonttc, bulej93, c3phas, cRat1st0s, csanuragjain, defsec, durianSausage, fatherOfBlocks, gogo, hake, hickuphh3, ignacio, joestakey, karanctf, kyteg, m_Rassska, pashov, rajatbeladiya, rbserver, robee, rokinot, samruna, sashik_eth, simon135, tofunmi
19.3779 USDC - $19.38
The casting of artIn to u128 is redundant in auction_.art -= artIn.u128(); because the subtraction auction_.art - artIn would have reverted when checking dust a couple of lines prior.
- auction_.art -= artIn.u128(); + // casting to u128 is redundant because it would have reverted in L438 + auction_.art -= artIn;
liquidatorCut -= auctioneerCut; can be unchecked because auctioneerReward <= 1e18, so we know auctioneerCut is guaranteed to be <= liquidatorCut
unchecked { liquidatorCut -= auctioneerCut; }