Platform: Code4rena
Start Date: 12/08/2022
Pot Size: $50,000 USDC
Total HM: 15
Participants: 120
Period: 5 days
Judge: Justin Goro
Total Solo HM: 6
Id: 153
League: ETH
Rank: 88/120
Findings: 1
Award: $45.83
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0x1f8b
Also found by: 0x52, 0xA5DF, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, EthLedger, Funen, IllIllI, JC, Junnon, Lambda, LeoS, MiloTruck, Noah3o6, PaludoX0, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, SaharAP, Sm4rty, SooYa, The_GUILD, TomJ, Waze, Yiko, _Adam, __141345__, a12jmx, ak1, asutorufos, auditor0517, ayeslick, ballx, beelzebufo, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, cccz, cryptonue, cryptphi, d3e4, delfin454000, dipp, djxploit, durianSausage, dy, erictee, fatherOfBlocks, gogo, gzeon, hyh, ignacio, kyteg, ladboy233, medikko, mics, minhquanym, oyc_109, pfapostol, rbserver, reassor, ret2basic, robee, sach1r0, simon135, sryysryy, tabish, yac, yash90, zzzitron
45.8342 USDC - $45.83
Overall, the code base was written quite well imo and had tons of helpful comments. Additionally, the corresponding architecture videos that the sponsor provided helped me get up to speed quite quickly.
Issue/Suggestion #1: Potentially consider adding the whenNotPaused modifier on core functions in FraxLendPairCore.
The whenNotPaused modifier exists on several core functions such as deposit, mint, borrowAsset, liquidate, liquidateClean, leveragedPosition, however is not visible on addCollateral and a few others.
My understanding is that the intention of the protocol and code is to protect borrowers. For example, in the case that the oracle price changes during a pause, borrowers are able to addCollateral to prevent liquidation. However, in the Aave lendingPool for example, it appears that all core functions contain the whenNotPaused modifier: https://github.com/aave/protocol-v2/blob/master/contracts/protocol/lendingpool/LendingPool.sol.
In the Aave case, all core functions are paused so if we consider the same case where the oracle price changes during a pause, there would be a race between the liquidator and the borrower to squeeze their transaction in first. So, this perhaps may not be desirable having the borrower and the liquidator on even footing grounds during a pause and unpause. I did want to mention this in my report so the team is aware and curious what others wardens/the judge think as well.
Issue/Suggestion #2: Potentially consider adding checks for known bad addresses (address(0), address(this), etc).
It is noteworthy that any incorrect function calls being passed with address(0) or address(this) will be lost in several functions. For example, calling the above function in the snippet with address(0) or address(this) will result in funds being lost. Again there are gas tradeoffs, etc here so this is ultimately up to the team. One could argue this would potentially be a better/safer user experience as it protects against any front-end errors or failure to validate addresses when calling any of the core functions.
Issue #3: Spelling Error here:
/// @dev Can only called once per block
should be
/// @dev Can only be called once per block
#0 - gititGoro
2022-10-06T00:42:22Z
This report was a little too verbose and the intro was not necessary. On the whenNotPaused modifier, the systemic risks Aave face are much higher than a pair based protocol such as this one.