Art Gobblers contest - bytehat's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L562

Vulnerability details

Impact

The updgradeRandProvider function exists in case the chainlink randomness provider is phased out in the long term future. It is impossible to upgrade the randomness provider while the contract is still waiting for a seed. This is a reasonable thing to implement, but it could cause a lot of problems. For example, it is possible that the chainlink randomness gets phased out while the protocol is still waiting for a seed, especially because requesting and consuming random seeds are permissionless functions. An attacker can pick the worst possible time to request a random seed (right before chainlink becomes deprecated in this scenario), perhaps by even front-running admin calls to upgradeRandProvider. This is a problem with the upgrading in general, perhaps the randomness provider after chainlink will have the same problem.

Proof of Concept

This is the line that could potentially brick the protocol: https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L562.

Tools Used

Manual

Recommended Mitigation Steps

There are two trains of thought:

1. Just delete the check entirely because it will probably cause more problems than it solves. The owner is already trusted to an extent to not upgrade to a malicious randomness provider, so it is not too much more to ask users to trust that the admins don't intentionally upgrade the rand provider while someone is waiting for a seed for some nefarious reasons.
2. Implement some sort of timer, if a call to the randomness provider has taken more than 1 week for example, then the admin can freely set the randomness provider even if a seed is still waiting to be processed. The problem with this is that it would require new code and this won't be checked that thoroughly unless you do another audit somewhere.