Platform: Code4rena
Start Date: 25/08/2022
Pot Size: $75,000 USDC
Total HM: 35
Participants: 147
Period: 7 days
Judge: 0xean
Total Solo HM: 15
Id: 156
League: ETH
Rank: 6/147
Findings: 3
Award: $2,817.16
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: enckrish
1905.4132 DAI - $1,905.41
https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L325
There are no checks to ascertain that the policy being removed is registered in the Kernel. Trying to remove a non-registered results in the policy registered at 0th index of activePolicies being removed.
https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L325
Adding require(activePolicies[idx] == policy_, "Unregistered policy"); will prevent this, where idx = getPolicyIndex[policy_].
NOTE: The issue is less likely to happen as this is handled solely by the executor, but having safeguards in the contract is always better than relying on an external factor.
#0 - ind-igo
2022-09-08T19:47:51Z
Confirmed. Should be lower risk or a QA issue.
#1 - 0xean
2022-09-19T22:17:33Z
@ind-igo - can you comment on why you think it should be QA vs Medium?
— Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
I would expect this to impact the functionality of the protocol.
#2 - Oighty
2022-09-22T16:13:54Z
This one seems on the fence to me. While accidentally unregistering a policy likely would affect the functionality of the protocol, it requires the executor to make a mistake. If the mistake is made, it's easily remedied by re-registering the policy.
#3 - 0xean
2022-09-22T16:50:17Z
That makes sense, but there would be some amount of down time when this occurred. I think M is correct for this issue.
857.4359 DAI - $857.44
https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L295
In Kernel._activatePolicy, no checks are done to ensure that the policy is not already registered in the Kernel contract. The check policy_.isActive() depends on an external call, and should be less preferred over contract's storage in this case. Successfully adding duplicate policy would result in failure when trying to deactivate it.
https://github.com/code-423n4/2022-08-olympus/blob/main/src/Kernel.sol#L295
Addition of the following code will mitigate this:
uint idx = getPolicyIndex[policy_]; if (idx != 0 || policy_ == activePolicies[0]) revert("Duplicate policy");
NOTE: The issue is less likely to happen as this is handled solely by the executor, but having safeguards in the contract is always better than relying on an external factor.
#0 - ind-igo
2022-09-02T20:47:46Z
This is simply not true. The policy isActive will cause it to revert if it gets activated again. Your suggestion was how the original code was, but was changed to allow more flexibility and less state management in the kernel.
#1 - 0xLienid
2022-09-08T02:22:52Z
duplicate of #52 @ind-igo issue #52 makes this sound possible actually
#2 - 0xean
2022-09-19T22:21:32Z
dupe of #52
🌟 Selected for report: zzzitron
Also found by: 0x040, 0x1f8b, 0x52, 0x85102, 0xDjango, 0xNazgul, 0xNineDec, 0xSky, 0xSmartContract, 0xkatana, 8olidity, Aymen0909, Bahurum, BipinSah, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chandr, Chom, CodingNameKiki, Deivitto, DimSon, Diraco, ElKu, EthLedger, Funen, GalloDaSballo, Guardian, IllIllI, JansenC, Jeiwan, Lambda, LeoS, Margaret, MasterCookie, PPrieditis, PaludoX0, Picodes, PwnPatrol, RaymondFam, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, StevenL, The_GUILD, TomJ, Tomo, Trust, Waze, __141345__, ajtra, ak1, apostle0x01, aviggiano, bin2chen, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, cccz, ch13fd357r0y3r, cloudjunky, cryptphi, csanuragjain, d3e4, datapunk, delfin454000, devtooligan, dipp, djxploit, durianSausage, eierina, enckrish, erictee, fatherOfBlocks, gogo, grGred, hansfriese, hyh, ignacio, indijanc, itsmeSTYJ, ladboy233, lukris02, martin, medikko, mics, natzuu, ne0n, nxrblsrpr, okkothejawa, oyc_109, p_crypt0, pfapostol, prasantgupta52, rajatbeladiya, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, sorrynotsorry, tnevler, tonisives, w0Lfrum, yixxas
54.3128 DAI - $54.31
EnumerableMap for Kernel.activePoliciesAn EnumerableMap like structure with mapping from Policy=>bool may be used for Kernel.activePolicies for preventing issues with duplicate prevention during development.