Art Gobblers contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

A function to change the RandProvider was deliberately provided because no current provider guarantees operation for 10 years. However, the new provider may not have a guarantee that every call will ultimately be fulfilled. Because of that, the current logic can result in situations where no reveals are possible anymore, which would brick the whole system.

Proof Of Concept

Because Chainlink's VRF v1 is soon sunset, Frankie decides to replace it with a VRF v2 RandProvider. However, because he is tired, he forgots to add funds to the subscription account. According to the Chainlink docs, the following will happen:

Each subscription must maintain a minimum balance to fund requests from consuming contracts. If your balance is below that minimum, your requests remain pending for up to 24 hours before they expire. After you add sufficient LINK to a subscription, pending requests automatically process as long as they have not expired.

Because the problem is noticed for 24 hours, the request will expire. Now, revealGobblers cannot be called anymore because gobblerRevealsData.waitingForSeed is still true. Moreover, it will never be possible to call it and the situation is unrecoverable. upgradeRandProvider & revealGobblers cannot be called (because of gobblerRevealsData.waitingForSeed) and requestRandomSeed can also not be called (because gobblerRevealsData.toBeRevealed > 0 and it is not possible to decrease this variable).

Other possibilities are:

• There is an outstanding request when VRF v1 is sunset: Even if the date is previously announced, someone could make updates impossible by contionusly requesting random data. In theory, this should be prevented by the nextRevealTimestamp, but if no reveals happened in a long time for some reason, it is still possible to continously request new data. For instance, if no reveals happened in a month, someone can call requestRandomSeed 30 times until the nextRevealTimestamp has catched up.
• The new RandProvider has a bug and never actually returns data.

Recommended Mitigation Steps

Implement an emergency function that lets you set a new provider (and reset gobblerRevealsData.toBeRevealed, even if there is an outstanding request). In practice, this should not be problematic. Even if a situation would arise where the old request is still answered, this is not problematic, as it will be declined (because calls are only allowed from the current randomness provider).

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/fb54f92ffcb0c13e72c84cde24c138866d9988e8/src/ArtGobblers.sol#L458 https://github.com/code-423n4/2022-09-artgobblers/blob/f3d4522ecfb6f02e6ca4ecd564d38e81d3021d4e/src/utils/token/GobblersERC721.sol#L166

Vulnerability details

Impact

In mintLegendaryGobbler, gobblersOwned is decreased by cost and then increased by 1 again to factor in the new legendary:

            // We subtract the amount of gobblers burned, and then add 1 to factor in the new legendary.
            getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost + 1);

However, _mint(msg.sender, gobblerId); is later called. This increases getUserData[msg.sender].gobblersOwned again:

            ++getUserData[to].gobblersOwned;

Because of that, the new legendary gobbler is counted two times. This will cause all subsequent balanceOf(user) calls to return wrong values (1 gobbler too high) and for the user, it will never be possible to have a balance below 1.

Proof Of Concept

--- a/test/ArtGobblers.t.sol
+++ b/test/ArtGobblers.t.sol
@@ -434,9 +434,10 @@ contract ArtGobblersTest is DSTestPlus {
         }
 
         assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum);
-
+        uint256 gobblersOwnedBefore = gobblers.balanceOf(users[0]);
         vm.prank(users[0]);
         uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);
+        assertEq(gobblers.balanceOf(users[0]), gobblersOwnedBefore - cost + 1); // an amount of cost gobblers are burned, 1 is minted...
 
         // Legendary is owned by user.
         assertEq(gobblers.ownerOf(mintedLegendaryId), users[0]);

Recommended Mitigation Steps

            // We subtract the amount of gobblers burned. The 1 for the legendary gobbler is added in _mint
            getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost);

Fisher Yates Shuffle Modulo Bias

In the Fisher Yates shuffle, there can be a very small modulo bias when choosing the distance: uint256 distance = randomSeed % remainingIds; For instance, let's say remainingIds is 9000. Because 2^64 mod 9000 = 3616, the distances {0, ..., 3615} have a likelihood that is increased by 4.9 * 10^(-16). In theory, the algorithm therefore does not perform a true random permutation (in the mathematical sense), but in practice, this should not be a problem. There might be some mint positions that are preferrable / where some ids are more likely by a very small amount.

Gobbler reveals can happen more often than every 24 hours

According to a comment in requestRandomSeed, reveals should only happen every 24 hours: // We want at most one batch of reveals every 24 hours. This is currently not always enforced. nextRevealTimestamp is always increased by 24 hours, not set to block.timestamp + 1 days. Because of that, multiple successive reveals are possible when no reveals happened for a long time. For instance, when no reveals happened for 30 days, requestRandomSeed can be called 30 times in a row (if enough Gobblers are minted such that always one can be revealed for every call).

Gobbling-as-a-Service

In my opinion, approved addresses should also be able to call gobble. This would allow other projects to integrate into Art Gobblers better. For instance, a protocol could offer GaaS, "Gobbling-as-a-service", where you let them manage the gobbling for your gobbler and they have portfolio managers or algorithms that carefully choose the art to feed to the gobbler in order to maximize his value. In 5 years, GaaS may be a huge industry and people want to become Gobbling quants and Gobbling portfolio managers ;)

LibGoo rounding causing checkpointing to affect result

Because of the rounding in LibGoo, it currently makes a (small) difference if you regularly checkpoint or not.

Consider the following diff, simulating a transfer to itself every 12 seconds:

--- a/test/ArtGobblers.t.sol
+++ b/test/ArtGobblers.t.sol
@@ -173,10 +173,18 @@ contract ArtGobblersTest is DSTestPlus {
         gobblers.mintFromGoo(type(uint256).max, true);
         //asert owner is correct
         assertEq(gobblers.ownerOf(2), users[0]);
+        uint startTime = block.timestamp;
         //asert balance went down by expected amount
-        uint256 finalBalance = gobblers.gooBalance(users[0]);
-        uint256 paidGoo = initialBalance - finalBalance;
-        assertEq(paidGoo, gobblerPrice);
+        // for (uint256 i = 0; i <= 86400 * 30 * 12; i += 12) {
+        //     vm.prank(users[0]);
+        //     gobblers.transferFrom(users[0], users[0], 2);
+        //     vm.warp(startTime + 3 days + i);
+        // }
+        vm.warp(startTime + 3 days + 86400 * 30 * 12);
+        console.log(gobblers.gooBalance(users[0]));
+        // uint256 finalBalance = gobblers.gooBalance(users[0]);
+        // uint256 paidGoo = initialBalance - finalBalance;
+        // assertEq(paidGoo, gobblerPrice);
     }

In theory, the logged number should be equal, no matter if we previously run the commented out lines (which transfer the token to oneself every 12 seconds for 12 months) or do not run it. In practice, the results are:

• With the token transfer every 12 seconds: 298319329386043864916293
• Without it: 298319329386047666906459 (a difference of 3801990166 tokens)

This will probably be very hard to avoid, but it is something to be aware of.