Platform: Code4rena
Start Date: 11/05/2022
Pot Size: $150,000 USDC
Total HM: 23
Participants: 93
Period: 14 days
Judge: LSDan
Total Solo HM: 18
Id: 123
League: ETH
Rank: 59/93
Findings: 2
Award: $233.12
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xNazgul, 0xNineDec, 0xf15ers, 0xkatana, 242, AlleyCat, BouSalman, BowTiedWardens, CertoraInc, Chom, Cityscape, FSchmoede, Funen, GimelSec, Hawkeye, JC, JDeryl, Kaiziron, Kthere, Kumpa, MaratCerby, MiloTruck, Nethermind, NoamYakov, PPrieditis, QuantumBrief, Rolezn, Ruhum, SmartSek, SooYa, Tadashi, TerrierLover, WatchPug, Waze, _Adam, asutorufos, berndartmueller, bobirichman, c3phas, catchup, cccz, ch13fd357r0y3r, cryptphi, csanuragjain, cthulhu_cult, defsec, delfin454000, ellahi, fatherOfBlocks, hansfriese, hubble, hyh, jayjonah8, joestakey, kenta, kenzo, kirk-baird, mics, oyc_109, p_crypt0, reassor, robee, sach1r0, samruna, sashik_eth, sikorico, simon135, sorrynotsorry, sseefried, tintin, unforgiven, z3s, zmj
149.8668 USDC - $149.87
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraClaimZap.sol#L95
In AuraClaimZap.sol the setApprovals() function makes use of safeApprove() six times. OpenZeppelin's safeApprove implementation is deprecated. Using this deprecated function can lead to unintended reverts and potentially the locking of funds.
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraClaimZap.sol#L95
https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219
Manual code review
Consider replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.
#0 - phijfry
2022-05-25T10:04:17Z
In all cases we are setting to 0 then setting to max. I don't believe there is any potential to become locked in the way described in this ticket.
#1 - 0xMaharishi
2022-05-25T16:22:24Z
All of the Aura safeApprove calls are either made in the constructor, or first set to 0 and then call again. I would say that this is valid although should be a 0 or 1
#2 - dmvt
2022-06-24T23:12:04Z
This holds as a QA issue. Deprecated code shouldn't be used ideally, but the issue will not lead to locking of funds or reverts. Even if they didn't notice a package upgrade or the like, the issue would present itself well before funds were locked.
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, BowTiedWardens, CertoraInc, DavidGialdi, FSchmoede, Fitraldys, Funen, GimelSec, Hawkeye, JC, Kaiziron, Kthere, MaratCerby, MiloTruck, NoamYakov, QuantumBrief, Randyyy, Ruhum, SmartSek, SooYa, Tadashi, TerrierLover, Tomio, UnusualTurtle, WatchPug, Waze, _Adam, antonttc, asutorufos, bobirichman, c3phas, catchup, csanuragjain, cthulhu_cult, defsec, delfin454000, ellahi, fatherOfBlocks, hansfriese, hyh, jayjonah8, joestakey, kenta, marcopaladin, mics, minhquanym, orion, oyc_109, reassor, rfa, robee, sach1r0, samruna, sashik_eth, sikorico, simon135, unforgiven, z3s, zmj
83.2512 USDC - $83.25
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/Aura.sol#L82
In Aura.sol the updateOperator() function can be called by anyone and it sets a new operator based on the address returned from IStaker(vecrvProxy).operator(). The problem is that anyone can call this function even if the operator on vecrvProxy is not yet set. If this is the case the operator in Aura.sol would be set to a zero address breaking the contract since functions like init() and mint() rely on the msg.sender being the operator. Even the minterMint() function relies on the operator since only the operator can set the minter which is the only one who can call minterMinter().
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/Aura.sol#L82
Manual code review
The updateOperator() function should not be a public function and should only be callable by an admin or the operator inside Aura.sol. Also in the updateOperator() function, there should be a check ensuring that the newOperator address is not a zero address to prevent breaking the contract by setting the operator to a zero address.
#0 - phijfry
2022-05-24T14:11:54Z
The voter proxy is already deployed and the operator is not set to address(0). So this can't happen.
#1 - 0xMaharishi
2022-05-25T16:18:41Z
This is certainly not a high severity issue, I would say a 1 at most.
The only problem that could happen here is if someone is watching the Aura deployment, and then calls updateOperator immediately after Aura has been deployed, but before the init fn has been called.
Solution is to add some protections.. a) checking system has been initialised and b) non zero address
#2 - dmvt
2022-06-20T17:51:39Z
Agree that this is not high severity. I'm going to downgrade it to gas, since the impact would be that they would need to redeploy the contracts. No fund loss is possible with this issue.