Open code423n4 opened 2 years ago
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?
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.
This warden did not submit a QA report so this will act as it
swivel, marketPlace and creator all have setAdmin
. vaulttracker sets the marketPlace at construction
Lines of code
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
Vulnerability details
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 byadmin
.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.