Platform: Code4rena
Start Date: 12/07/2022
Pot Size: $35,000 USDC
Total HM: 13
Participants: 78
Period: 3 days
Judge: 0xean
Total Solo HM: 6
Id: 135
League: ETH
Rank: 66/78
Findings: 1
Award: $44.25
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: joestakey
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xNineDec, 8olidity, Avci, Bahurum, Bnke0x0, Chom, ElKu, Funen, GimelSec, JC, Junnon, Kaiziron, Meera, PaludoX0, Picodes, ReyAdmirado, Sm4rty, Soosh, Waze, _Adam, __141345__, ak1, aysha, benbaessler, bin2chen, c3phas, cccz, cryptphi, csanuragjain, defsec, exd0tpy, fatherOfBlocks, gogo, hake, hansfriese, itsmeSTYJ, jonatascm, kyteg, mektigboy, oyc_109, pashov, rbserver, rishabh, robee, rokinot, sach1r0, sashik_eth, scaraven, simon135, slywaters
44.2547 USDC - $44.25
https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/Creator.sol#L47 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L53 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L428
Proof of concept: The current admin transfer pattern is calling setAdmin() function with an address type argument, which is immediately set to the new admin of the contract. If the new address of the new admin is actually not a valid account then the admin just got transferred to an uncontrolled account, breaking all functions that can be called only by admin.
Impact: this can impact availability of the protocol
Recommended Mitigation Steps: Implement a two-step process where the admin nominates an account and the nominated account needs to call an acceptAdmin() function for the transfer of admin to be completed. This ensures the nominated account is a valid & active one.
#0 - JTraversa
2022-07-20T07:43:18Z
I've heard more folks recommend this sort of pattern recently but I'd really not consider it a med-risk, prob a potential QA suggestion?
#1 - bghughes
2022-08-03T02:06:40Z
I've heard more folks recommend this sort of pattern recently but I'd really not consider it a med-risk, prob a potential QA suggestion?
Yes, I agree that this should be QA. Centralization risk is acknowledged and this is a "nice-to-have" sanity check IMO.
#2 - bghughes
2022-08-03T02:07:16Z
This warden did not submit a QA report so this will act as it
#3 - robrobbins
2022-08-24T18:29:56Z
swivel, marketPlace and creator all have setAdmin. vaulttracker sets the marketPlace at construction