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: 5/120
Findings: 3
Award: $2,774.28
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: auditor0517
Also found by: 0xA5DF, _Adam, cccz, minhquanym, minhtrng, zzzitron
192.5076 USDC - $192.51
After confirmed with the sponsor, dirtyLiquidationFee is 90% of cleanLiquidationFee like the comment.
But it uses 9% (9000 / 1e5 = 0.09) and the fee calculation will be wrong here.
Manual Review
We should change 9000 to 90000.
dirtyLiquidationFee = (_liquidationFee * 90000) / LIQ_PRECISION; // 90% of clean fee
#0 - 0xA5DF
2022-08-17T20:43:33Z
Duplicate of #132
#1 - DrakeEvans
2022-09-06T13:32:32Z
Confirmed
#2 - gititGoro
2022-09-27T05:51:21Z
This issue has a great many duplicates. Reason for setting this report as the original: 1.It's complete in that it charts the problem and gives a code based mitigation 2. It's short and to the point. 3. The sponsor's preference was factored in.
🌟 Selected for report: auditor0517
2535.6587 USDC - $2,535.66
This function is changing the protocol fee that is used during interest calculation here.
But it doesn't update interest before changing the fee so the _feesAmount will be calculated wrongly.
As we can see during pause() and unpause(), _addInterest() must be called before any changes.
But with the changeFee(), it doesn't update interest and the _feesAmount might be calculated wrongly.
T1, _currentRateInfo.feeToProtocolRate = F1.T2, the owner had changed the fee to F2.T3, _addInterest() is called during deposit() or other functions.F1 should be applied from T1 to T2 and F2 should be applied from T2 and T3. But it uses F2 from T1 to T2.Manual Review
Recommend modifying changeFee() like below.
function changeFee(uint32 _newFee) external whenNotPaused { if (msg.sender != TIME_LOCK_ADDRESS) revert OnlyTimeLock(); if (_newFee > MAX_PROTOCOL_FEE) { revert BadProtocolFee(); } _addInterest(); //+++++++++++++++++++++++++++++++++ currentRateInfo.feeToProtocolRate = _newFee; emit ChangeFee(_newFee); }
#0 - DrakeEvans
2022-09-06T13:34:07Z
Disagree with severity as it can be mitigated by calling addInterest() beforehand by admin or regular user. But we will address it anyway for convenience. We assume admins are not malicious.
#1 - 0xA5DF
2022-09-06T14:41:56Z
We assume admins are not malicious.
Admins might not be malicious, but without knowing about the issue (i.e. if the warden hasn't reported this) they wouldn't call addInterest() beforehand, leading to higher interest than intended.
#2 - gititGoro
2022-09-30T04:16:25Z
Maintaining severity as it is a potential leakage.
🌟 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
46.1121 USDC - $46.11
_deltaTime in the future.As we can see from below links, _currentRateInfo.lastTimestamp saves uint64(block.timestamp) but uses raw block.timestamp for _deltaTime.
File: 2022-08-frax\src\contracts\FraxlendPairCore.sol 441: uint256 _deltaTime = block.timestamp - _currentRateInfo.lastTimestamp;
File: 2022-08-frax\src\contracts\FraxlendPairCore.sol 464: _currentRateInfo.lastTimestamp = uint64(block.timestamp);
It will be wrong when block.timestamp > type(uint64).max more than 584 * 10^9 years later.
I submit as a low risk because the time is impractical.
We can calculate _deltaTime like this.
uint256 _deltaTime = uint256(uint64(uint64(block.timestamp) - _currentRateInfo.lastTimestamp));
lastTimestamp properly.As we can see from below links, _exchangeRateInfo.lastTimestamp saves uint32(block.timestamp) but compares with block.timestamp for same timestamp.
So after the 2106 year, this condition will be always false and it will update the rate every time.
File: 2022-08-frax\src\contracts\FraxlendPairCore.sol 518: if (_exchangeRateInfo.lastTimestamp == block.timestamp) { 519: return _exchangeRate = _exchangeRateInfo.exchangeRate; 520: }
File: 2022-08-frax\src\contracts\FraxlendPairCore.sol 544: _exchangeRateInfo.lastTimestamp = uint32(block.timestamp);
We should modify L518 like below.
if (_exchangeRateInfo.lastTimestamp == uint32(block.timestamp)) {
#0 - gititGoro
2022-10-06T01:28:09Z
584 billion years is about 64 times the duration of the Sun's main sequence.