Art Gobblers contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L441

Vulnerability details

Impact

Users can recover already burned gobblers after minting a legendary gobbler.

The main flaw is that it doesn't reset getApproved[id] here.

As a result, users can have more emissionMultiple than they should by recovering the burned gobblers.

Proof of Concept

The below attack scenario is possible.

• Alice has certain number of gobblers like 50 and she sets getApproved[id] = Alice for her gobblers here.
• After that, she mints a legendary gobbler by burning her gobblers.
• But getApproved[id] will be remaining here.
• Alice transfers already burned NFTs to herself using transferFrom() with from = address(0), to = Alice.
• Then this one will be passed properly because the owner of the burned NFT is address(0).
• Also, these calculations will work because they are inside the unchecked block.
• Finally, she can receive emissionMultiple again from the burned tokens here and she will get more GOO tokens.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should delete getApproved[id] here.

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L546

Vulnerability details

Impact

ArtGobblers contract wouldn't receive a random seed forever by a malicious user.

Currently it can't execute revealGobblers() and upgradeRandProvider() when gobblerRevealsData.waitingForSeed == true.

So if the contract fails to receive the random seed after requesting it, waitingForSeed will be true forever and the whole logic will be messy because the contract can't reveal new gobblers.

Proof of Concept

• As we can see from this document, VRF V1 always waited for 10 blocks on Ethereum before delivering on-chain randomness.

More configuration capability: 
VRF V1 always waited for 10 blocks on Ethereum before delivering on-chain randomness.

• And from this article, the VRF service will not attempt to call it a second time.

If your fulfillRandomness implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert. Consider simply storing the randomness and taking more complex follow-on actions in separate contract calls made by you or your users.

• This article shows it's possible to prevent other transactions for the small number of blocks with a high gas price.
• So if a malicious user prevents other txs at most 10 blocks after the ArtGobblers contract requested a new random seed, gobblerRevealsData.waitingForSeed will be true permanently and the contract can't reveal NFTs.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we should reset gobblerRevealsData.waitingForSeed = false in upgradeRandProvider() so that the admin can upgrade a RandProvider when there is a problem with the previous one even though gobblerRevealsData.waitingForSeed == true.

function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
    
    gobblerRevealsData.waitingForSeed = false;

    randProvider = newRandProvider; // Update the randomness provider.

    emit RandProviderUpgraded(msg.sender, newRandProvider);
}

Otherwise, we can add a new function like ResetWaitingForSeed() to reset gobblerRevealsData.waitingForSeed = false.

function ResetWaitingForSeed() external onlyOwner {
    gobblerRevealsData.waitingForSeed = false;
}

[N-01] Use two-phase ownership transfers

• https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/auth/Owned.sol#L39
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L83
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/GobblerReserve.sol#L12

ArtGobblers and GobblerReserve contracts inherite the Owned contract and use setOwner() function without any two-phase ownership transfers.

[N-02] Immutable addresses should be 0-checked

• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L314-L317
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/Pages.sol#L179-L181
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/GobblerReserve.sol#L24
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/rand/ChainlinkV1RandProvider.sol#L55

[N-03] Event is missing indexed fields

• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L233-L234
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L240
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/Pages.sol#L134-L136
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/token/GobblersERC721.sol#L17
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/token/GobblersERC1155B.sol#L29
• https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/token/PagesERC721.sol#L18

Each event should use three indexed fields if there are three or more fields