Platform: Code4rena
Start Date: 21/06/2022
Pot Size: $55,000 USDC
Total HM: 29
Participants: 88
Period: 5 days
Judge: gzeon
Total Solo HM: 7
Id: 134
League: ETH
Rank: 31/88
Findings: 2
Award: $270.73
🌟 Selected for report: 0
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L255
Suppose you tried to pause the lend function that is connected to Swivel. In that case, the attacker could sidestep it because there is no verification that the number input from the user corresponds with the desired input.
Let's say a bug is found, and you pause the lend function that interacts with Swivel. The attacker could call the lend function and, for the p argument, pass any number > 10 and < 256. Then the modifier checks the mapping, but since all uninitialized numbers return false, the modifier will pass and not revert.
This wouldn't be a problem if you validated that the p input corresponds with the Swivel number in the mapping. But since you don't validate it, an attacker can pass in any number, thus always pass the modifier.
This bug is only in the one lend function since, in others, you either validate the input or call the marketplace.sol with the p argument, which will return the 0 address, so the function won't do anything.
Manual Review
Validate that the p input corresponds with the number in the mapping.
I recommend that you validate it in all lend functions to ensure there is no unexpected behavior.
#0 - KenzoAgada
2022-06-28T06:33:00Z
Duplicate of #343
🌟 Selected for report: defsec
Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xkowloon, 0xmint, Bnke0x0, BowTiedWardens, Chom, ElKu, Funen, GalloDaSballo, GimelSec, IllIllI, JC, Kenshin, Kulk0, Lambda, Limbooo, MadWookie, Metatron, Picodes, Soosh, StErMi, TomJ, WatchPug, Waze, Yiko, _Adam, ak1, asutorufos, aysha, bardamu, catchup, datapunk, delfin454000, dipp, fatherOfBlocks, grGred, hake, hansfriese, hyh, joestakey, kebabsec, kenzo, kirk-baird, oyc_109, pashov, poirots, rfa, robee, saian, sashik_eth, shenwilly, simon135, slywaters, z3s, zeesaw, zer0dot
63.9425 USDC - $63.94
In Lender.setAdmin function, you set the new admin without the two-step process. If, because of a bug or a mistake, wrong address is passed in, you could lose access to the contract forever.
Trail of bits themselves recommend this strategy. You can look it up in this report: https://github.com/trailofbits/publications/blob/master/reviews/MapleFinance.pdf on page 17. Bear in mind that the circumstances are different, but the general idea is the same.
Github Link: https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L129
I know that attacks that are viable only when the private keys are compromised are out of scope, but I figured I would include it to be sure.
In Lender.approve, the admin can approve anybody for the max amount of tokens. If the keys were compromised, the attacker could drain the contract.
It would be safer to add some restrictions.