Art Gobblers contest - devtooligan's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L509-L538

Vulnerability details

Impact

ArtGobblers incorporates Chainlink VRF v1 which uses off-chain, centralized processes to choose a random number which is then verified on-chain. ArtGobblers also contains a permissioned upgradeRandProvider() which can be used to change the rand provider contract. However, the upgrade cannot be called while waiting for a random seed (after calling requestRandomSeed() but before fulfillRandomness() has been called).

If upgradeRandProvider were called with the wrong address and subsequently requestRandomSeed() was called, or if a problem with Chainlink VRF v1 were to arise that prevented Chainlink from calling fullfillRandomness() while waiting for the random seed, then the reveal process could be irrecoverably bricked. upgradeRandProvider cannot save us because of the check:

  if (gobblerRevealsData.waitingForSeed) revert SeedPending();

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L560-L567

Recommended Mitigation Steps

Consider adding an additional bool arg to upgradeRandProvider() which would reset waitingForSeed to false and toBeRevealed to zero. This would allow for optionally (and explicitly) resetting the waiting state of the contract and prevent it from being permanently bricked.

@@ -557,9 +560,14 @@ contract ArtGobblers is GobblersERC721, LogisticVRGDA, Owned, ERC1155TokenReceiv

     /// @notice Upgrade the rand provider contract. Useful if current VRF is sunset.
     /// @param newRandProvider The new randomness provider contract address.
-    function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
+    function upgradeRandProvider(RandProvider newRandProvider, bool resetWaiting) external onlyOwner {
+        if (resetWaiting) {
+            gobblerRevealsData.waitingForSeed = false;
+            gobblerRevealsData.toBeRevealed = 0;
+        }
+

QA Report

Variable used before assignment causing erroneous event emmited

Severity: Low

In requestRandomBytes() in ChainlinkV1RandProvider.sol, requestId is set as the return variable. It is then used (before being assigned a value) to emit the RandomBytesRequested event. On the following line, an explicit return is used.

    /// @notice Request random bytes from Chainlink VRF. Can only by called by the ArtGobblers contract.
    function requestRandomBytes() external returns (bytes32 requestId) {
        // The caller must be the ArtGobblers contract, revert otherwise.
        if (msg.sender != address(artGobblers)) revert NotGobblers();

        emit RandomBytesRequested(requestId);

        // Will revert if we don't have enough LINK to afford the request.
        return requestRandomness(chainlinkKeyHash, chainlinkFee);
    }

Impact

An incorrect event is emitted and logged permanently.

Recommendation: Fix it

@@ -63,10 +63,9 @@ contract ChainlinkV1RandProvider is RandProvider, VRFConsumerBase {
         // The caller must be the ArtGobblers contract, revert otherwise.
         if (msg.sender != address(artGobblers)) revert NotGobblers();

-        emit RandomBytesRequested(requestId);
-
         // Will revert if we don't have enough LINK to afford the request.
-        return requestRandomness(chainlinkKeyHash, chainlinkFee);
+        emit RandomBytesRequested(requestId = requestRandomness(chainlinkKeyHash, chainlinkFee));
+
     }

---

Attacker can spam mintReservedGobblers for an annoying DoS attack

Severity: Low

mintReservedGobblers() is a non-permissioned external function that can be called by anyone. If the function were called with 0, it would complete successfully, including a emitting an event at the end.

Impact This would mostly just be annoying. If front end was listening to this event, it could possibly have an effect. It would clutter up the recent events and event log queries.

Recommendation:

@@ -836,7 +828,9 @@ contract ArtGobblers is GobblersERC721, LogisticVRGDA, Owned, ERC1155TokenReceiv
     /// the supply of goo minted gobblers and the supply of gobblers minted to reserves.
     function mintReservedGobblers(uint256 numGobblersEach) external returns (uint256 lastMintedGobblerId) {
+        require(numGobblersEach > 0, "CAN'T MINT ZERO");

---

Use a less buggy version of Solidity

Severity: Low

foundry.toml has the compiler set to 0.8.13.

Solidity versions 0.8.15 - 0.8.17 fix several bugs introduced in earlier versions of Solidity, 2 of which were introduced in 0.8.13.

Impact

None of the fixed bugs were deemed to directly effect the ArtGobblers contracts.

Recommendation:

It is recommended to upgrade to the latest version of Solidity, or else take a more conservative approach and use an older version such as 0.8.4 which includes many needed features (but possibly less optimizations), but has been around longer and can be deemed more stable.