Platform: Code4rena
Start Date: 20/09/2022
Pot Size: $100,000 USDC
Total HM: 4
Participants: 109
Period: 7 days
Judge: GalloDaSballo
Id: 163
League: ETH
Rank: 1/109
Findings: 4
Award: $9,175.91
š Selected for report: 2
š Solo Findings: 0
š Selected for report: minhtrng
Also found by: 0xSmartContract, IllIllI, rbserver
6661.6008 USDC - $6,661.60
https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L560-L567 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/GobblerReserve.sol#L34-L41
There are a few ways that an owner can rug protocol users.
Even if the owner is benevolent, the fact that there is a rug vector available may negatively impact the protocol's reputation.
The owner can set a non-random source of randomness:
File: /src/ArtGobblers.sol #1 560 function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner { 561 // Revert if waiting for seed, so we don't interrupt requests in flight. 562 if (gobblerRevealsData.waitingForSeed) revert SeedPending(); 563 564 randProvider = newRandProvider; // Update the randomness provider. 565 566 emit RandProviderUpgraded(msg.sender, newRandProvider); 567: }
The owner can specify whichever addresses they wish when distributing from the GobblerReserve:
File: /src/utils/GobblerReserve.sol #2 34 function withdraw(address to, uint256[] calldata ids) external onlyOwner { 35 // This is quite inefficient, but it's okay because this is not a hot path. 36 unchecked { 37 for (uint256 i = 0; i < ids.length; i++) { 38 artGobblers.transferFrom(address(this), to, ids[i]); 39 } 40 } 41: }
Code inspection
Document known risks
#0 - Shungy
2022-09-28T12:03:21Z
First one is duplicate of: https://github.com/code-423n4/2022-09-artgobblers-findings/issues/327 Second one is duplicate of: https://github.com/code-423n4/2022-09-artgobblers-findings/issues/431
I believe the severity for the first one is not justified, and the second one is invalid. Reasoning stated in the comments of the above issues.
#1 - GalloDaSballo
2022-09-29T20:51:16Z
Will bulk as dup for Randomness Provider
#2 - GalloDaSballo
2022-09-29T20:51:46Z
š Selected for report: zzykxx
Also found by: 8olidity, IllIllI, Lambda, berndartmueller, bytehat, devtooligan, hansfriese, imare, obront, philogy, shung, tonisives, zdhu, zkhorse
470.3582 USDC - $470.36
Most of the README.md reads as though after the initial setup, the project will be mostly hands off. Given this, it's possible that the busy developers of this project may forget that the RandProvider needs to be upgraded. It's also possible that Chainlink misses an update due to a bug.
If the deprecated Chainlink VRF V1 RandProvider is not updated in time, or there is a bug in Chainlink that prevents it from responding to the specific request, it's possible that a new seed is requested right before Chainlink finally turns off the endpoint/has the bug, and therefore the ArtGobblers contract will never get the seed it's waiting for. The contract does not allow the changing of the provider unless there are no outstanding requests, so if this scenario happens, no more gobblers will be revealed, and users that spent money and Goo to mint them will be unable to get any benefit from having done so.
File: /src/ArtGobblers.sol #1 560 function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner { 561 // Revert if waiting for seed, so we don't interrupt requests in flight. 562 if (gobblerRevealsData.waitingForSeed) revert SeedPending(); 563 564 randProvider = newRandProvider; // Update the randomness provider. 565 566 emit RandProviderUpgraded(msg.sender, newRandProvider); 567: }
Code inspection
Allow the changing of the RandProvider, or the requesting of a new seed, if the seed has not come within a certain period of time
#0 - Shungy
2022-09-27T17:48:46Z
#1 - GalloDaSballo
2022-10-09T21:57:04Z
Dup of #153
#2 - GalloDaSballo
2022-10-09T21:57:27Z
Conditionality is most likely med
š Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0x52, 0x5rings, 0xNazgul, 0xRobocop, 0xSmartContract, 0xdeadbeef, 0xsanson, 8olidity, Amithuddar, Aymen0909, B2, B353N, CertoraInc, Ch_301, Chom, CodingNameKiki, Deivitto, ElKu, Funen, JC, JohnnyTime, Kresh, Lambda, Noah3o6, RaymondFam, ReyAdmirado, RockingMiles, Rolezn, Sm4rty, SuldaanBeegsi, Tadashi, TomJ, Tomio, V_B, Waze, __141345__, a12jmx, ak1, arcoun, asutorufos, aviggiano, berndartmueller, bharg4v, bin2chen, brgltd, bulej93, c3phas, catchup, cccz, ch0bu, cryptonue, cryptphi, csanuragjain, delfin454000, devtooligan, djxploit, durianSausage, eighty, erictee, exd0tpy, fatherOfBlocks, giovannidisiena, hansfriese, ignacio, joestakey, ladboy233, lukris02, m9800, malinariy, martin, minhtrng, obront, oyc_109, pedr02b2, pedroais, pfapostol, philogy, prasantgupta52, rbserver, ronnyx2017, rotcivegaf, rvierdiiev, sach1r0, shung, simon135, throttle, tnevler, tonisives, wagmi, yixxas, zkhorse, zzykxx, zzzitron
905.9642 USDC - $905.96
| Issue | Instances | |
|---|---|---|
| [Lā01] | Don't roll your own crypto | 1 |
| [Lā02] | Chainlink's VRF V1 is deprecated | 1 |
| [Lā03] | Rinkbe is not supported for Chainlink's VRF | 1 |
| [Lā04] | Missing checks for address(0x0) when assigning values to address state variables | 10 |
Total: 13 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | requestId is always zero | 1 |
| [Nā02] | Don't use periods with fragments | 1 |
| [Nā03] | Contract implements interface without extending the interface | 1 |
| [Nā04] | public functions not called by the contract should be declared external instead | 1 |
| [Nā05] | constants should be defined rather than using magic numbers | 112 |
| [Nā06] | Use a more recent version of solidity | 2 |
| [Nā07] | Use a more recent version of solidity | 2 |
| [Nā08] | Constant redefined elsewhere | 8 |
| [Nā09] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 3 |
| [Nā10] | File is missing NatSpec | 2 |
| [Nā11] | NatSpec is incomplete | 10 |
| [Nā12] | Event is missing indexed fields | 16 |
| [Nā13] | Not using the named return variables anywhere in the function is confusing | 3 |
| [Nā14] | Duplicated require()/revert() checks should be refactored to a modifier or function | 2 |
Total: 164 instances over 14 issues
The general advice when it comes to cryptography is don't roll your own crypto. The Chainlink VRF best practices page says that in order to get multiple values from a single random value, one should hash a counter along with the random value. This project does not follow that guidance, and instead recursively hashes previous hashes. Logically, if you have a source of entropy, then truncate, and hash again, over and over, you're going to lose entropy if there's a weakness in the hashing function, and every hashing function will eventually be found to have a weakness. Unless there is a very good reason not to, the code should follow the best practice Chainlink outlines. Furthermore, the current scheme wastes gas updating the random seed in every function call, as is outlined in my separate gas report.
There is 1 instance of this issue:
File: /src/ArtGobblers.sol 664 // Update the random seed to choose a new distance for the next iteration. 665 // It is critical that we cast to uint64 here, as otherwise the random seed 666 // set after calling revealGobblers(1) thrice would differ from the seed set 667 // after calling revealGobblers(3) a single time. This would enable an attacker 668 // to choose from a number of different seeds and use whichever is most favorable. 669 // Equivalent to randomSeed = uint64(uint256(keccak256(abi.encodePacked(randomSeed)))) 670 assembly { 671 mstore(0, randomSeed) // Store the random seed in scratch space. 672 673 // Moduloing by 1 << 64 (2 ** 64) is equivalent to a uint64 cast. 674 randomSeed := mod(keccak256(0, 32), shl(64, 1)) 675 } 676 } 677 678 // Update all relevant reveal state. 679: gobblerRevealsData.randomSeed = uint64(randomSeed);
VRF V1 is deprecated, so new projects should not use it
There is 1 instance of this issue:
File: /src/utils/rand/ChainlinkV1RandProvider.sol 13 /// @notice RandProvider wrapper around Chainlink VRF v1. 14: contract ChainlinkV1RandProvider is RandProvider, VRFConsumerBase {
Rinkeby is deprecated and not listed as supported for Chainlink. The documentation states Once the Ethereum mainnet transitions to proof-of-stake, Rinkeby will no longer be an accurate staging environment for mainnet. ... Developers who currently use Rinkeby as a staging/testing environment should prioritize migrating to Goerli or Sepolia link
There is 1 instance of this issue:
File: /script/deploy/DeployRinkeby.s.sol 6: contract DeployRinkeby is DeployBase {
address(0x0) when assigning values to address state variablesThere are 10 instances of this issue:
File: src/ArtGobblers.sol 316: team = _team; 317: community = _community;
File: script/deploy/DeployBase.s.sol 49: teamColdWallet = _teamColdWallet; 52: vrfCoordinator = _vrfCoordinator; 53: linkToken = _linkToken;
File: src/Pages.sol 181: community = _community;
File: src/Goo.sol 83: artGobblers = _artGobblers; 84: pages = _pages;
File: lib/solmate/src/auth/Owned.sol 30: owner = _owner; 40: owner = newOwner;
requestId is always zeroChainlink suggests to have a unique requestId for every separate randomness request. By always using the same value, it's not possible to tell whether Chainlink returned data for a valid request, or if there was some Chainlink bug that triggered a callback for a request that was never made
There is 1 instance of this issue:
File: /src/utils/rand/ChainlinkV1RandProvider.sol 62 function requestRandomBytes() external returns (bytes32 requestId) { 63 // The caller must be the ArtGobblers contract, revert otherwise. 64 if (msg.sender != address(artGobblers)) revert NotGobblers(); 65 66: emit RandomBytesRequested(requestId);
Throughout the files, most of the comments have fragments that end with periods. They should either be converted to actual sentences with both a noun phrase and a verb phrase, or the periods should be removed.
There is 1 instance of this issue:
File: /src/ArtGobblers.sol
Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact
There is 1 instance of this issue:
File: src/utils/token/GobblersERC1155B.sol /// @audit IERC1155MetadataURI.balanceOf(), IERC1155MetadataURI.uri(), IERC1155MetadataURI.safeTransferFrom(), IERC1155MetadataURI.safeBatchTransferFrom(), IERC1155MetadataURI.balanceOfBatch() 8: abstract contract GobblersERC1155B {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There is 1 instance of this issue:
File: lib/goo-issuance/src/LibGOO.sol 17 function computeGOOBalance( 18 uint256 emissionMultiple, 19 uint256 lastBalanceWad, 20 uint256 timeElapsedWad 21: ) public pure returns (uint256) {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 112 instances of this issue:
File: src/ArtGobblers.sol /// @audit 69.42e18 304: 69.42e18, // Target price. /// @audit 0.31e18 305: 0.31e18, // Price decay percent. /// @audit 0.0023e18 308: 0.0023e18 // Time scale. /// @audit 9 632: uint256 newCurrentIdMultiple = 9; // For beyond 7963. /// @audit 5 848: if (newNumMintedForReserves > (numMintedFromGoo + newNumMintedForReserves) / 5) revert ReserveImbalance();
File: lib/solmate/src/utils/FixedPointMathLib.sol /// @audit 0x10000000000000000000000000000000000 177: if iszero(lt(y, 0x10000000000000000000000000000000000)) { /// @audit 0x1000000000000000000 181: if iszero(lt(y, 0x1000000000000000000)) { /// @audit 0x10000000000 185: if iszero(lt(y, 0x10000000000)) { /// @audit 0x1000000 189: if iszero(lt(y, 0x1000000)) {
File: lib/solmate/src/tokens/ERC721.sol /// @audit 0x01ffc9a7 148: interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 /// @audit 0x80ac58cd 149: interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721 /// @audit 0x5b5e139f 150: interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata
File: src/utils/token/GobblersERC1155B.sol /// @audit 0x01ffc9a7 126: interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 /// @audit 0xd9b67a26 127: interfaceId == 0xd9b67a26 || // ERC165 Interface ID for ERC1155 /// @audit 0x0e89341c 128: interfaceId == 0x0e89341c; // ERC165 Interface ID for ERC1155MetadataURI
File: lib/solmate/src/utils/SignedWadMath.sol /// @audit 42139678854452767551 86: if (x <= -42139678854452767551) return 0; /// @audit 135305999368893231589 90: if (x >= 135305999368893231589) revert("EXP_OVERFLOW"); /// @audit 78 /// @audit 5 /// @audit 18 95: x = (x << 78) / 5**18; /// @audit 96 /// @audit 54916777467707473351141471128 /// @audit 95 /// @audit 96 100: int256 k = ((x << 96) / 54916777467707473351141471128 + 2**95) >> 96; /// @audit 54916777467707473351141471128 101: x = x - k * 54916777467707473351141471128; /// @audit 1346386616545796478920950773328 107: int256 y = x + 1346386616545796478920950773328; /// @audit 96 /// @audit 57155421227552351082224309758442 108: y = ((y * x) >> 96) + 57155421227552351082224309758442; /// @audit 94201549194550492254356042504812 109: int256 p = y + x - 94201549194550492254356042504812; /// @audit 96 /// @audit 28719021644029726153956944680412240 110: p = ((p * y) >> 96) + 28719021644029726153956944680412240; /// @audit 4385272521454847904659076985693276 /// @audit 96 111: p = p * x + (4385272521454847904659076985693276 << 96); /// @audit 2855989394907223263936484059900 114: int256 q = x - 2855989394907223263936484059900; /// @audit 96 /// @audit 50020603652535783019961831881945 115: q = ((q * x) >> 96) + 50020603652535783019961831881945; /// @audit 96 /// @audit 533845033583426703283633433725380 116: q = ((q * x) >> 96) - 533845033583426703283633433725380; /// @audit 96 /// @audit 3604857256930695427073651918091429 117: q = ((q * x) >> 96) + 3604857256930695427073651918091429; /// @audit 96 /// @audit 14423608567350463180887372962807573 118: q = ((q * x) >> 96) - 14423608567350463180887372962807573; /// @audit 96 /// @audit 26449188498355588339934803723976023 119: q = ((q * x) >> 96) + 26449188498355588339934803723976023; /// @audit 3822833074963236453042738258902158003155416615667 /// @audit 195 136: r = int256((uint256(r) * 3822833074963236453042738258902158003155416615667) >> uint256(195 - k)); /// @audit 0xffffffffffffffffffffffffffffffff 150: r := shl(7, lt(0xffffffffffffffffffffffffffffffff, x)) /// @audit 0xffffffffffffffff 151: r := or(r, shl(6, lt(0xffffffffffffffff, shr(r, x)))) /// @audit 0xffffffff 152: r := or(r, shl(5, lt(0xffffffff, shr(r, x)))) /// @audit 0xffff 153: r := or(r, shl(4, lt(0xffff, shr(r, x)))) /// @audit 0xff 154: r := or(r, shl(3, lt(0xff, shr(r, x)))) /// @audit 0xf 155: r := or(r, shl(2, lt(0xf, shr(r, x)))) /// @audit 0x3 156: r := or(r, shl(1, lt(0x3, shr(r, x)))) /// @audit 0x1 157: r := or(r, lt(0x1, shr(r, x))) /// @audit 96 162: int256 k = r - 96; /// @audit 159 163: x <<= uint256(159 - k); /// @audit 159 164: x = int256(uint256(x) >> 159); /// @audit 3273285459638523848632254066296 168: int256 p = x + 3273285459638523848632254066296; /// @audit 96 /// @audit 24828157081833163892658089445524 169: p = ((p * x) >> 96) + 24828157081833163892658089445524; /// @audit 96 /// @audit 43456485725739037958740375743393 170: p = ((p * x) >> 96) + 43456485725739037958740375743393; /// @audit 96 /// @audit 11111509109440967052023855526967 171: p = ((p * x) >> 96) - 11111509109440967052023855526967; /// @audit 96 /// @audit 45023709667254063763336534515857 172: p = ((p * x) >> 96) - 45023709667254063763336534515857; /// @audit 96 /// @audit 14706773417378608786704636184526 173: p = ((p * x) >> 96) - 14706773417378608786704636184526; /// @audit 795164235651350426258249787498 /// @audit 96 174: p = p * x - (795164235651350426258249787498 << 96); /// @audit 5573035233440673466300451813936 178: int256 q = x + 5573035233440673466300451813936; /// @audit 96 /// @audit 71694874799317883764090561454958 179: q = ((q * x) >> 96) + 71694874799317883764090561454958; /// @audit 96 /// @audit 283447036172924575727196451306956 180: q = ((q * x) >> 96) + 283447036172924575727196451306956; /// @audit 96 /// @audit 401686690394027663651624208769553 181: q = ((q * x) >> 96) + 401686690394027663651624208769553; /// @audit 96 /// @audit 204048457590392012362485061816622 182: q = ((q * x) >> 96) + 204048457590392012362485061816622; /// @audit 96 /// @audit 31853899698501571402653359427138 183: q = ((q * x) >> 96) + 31853899698501571402653359427138; /// @audit 96 /// @audit 909429971244387300277376558375 184: q = ((q * x) >> 96) + 909429971244387300277376558375; /// @audit 1677202110996718588342820967067443963516166 201: r *= 1677202110996718588342820967067443963516166; /// @audit 16597577552685614221487285958193947469193820559219878177908093499208371 203: r += 16597577552685614221487285958193947469193820559219878177908093499208371 * k; /// @audit 600920179829731861736702779321621459595472258049074101567377883020018308 205: r += 600920179829731861736702779321621459595472258049074101567377883020018308; /// @audit 174 207: r >>= 174;
File: src/utils/token/GobblersERC721.sol /// @audit 0x01ffc9a7 151: interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 /// @audit 0x80ac58cd 152: interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721 /// @audit 0x5b5e139f 153: interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata
File: src/utils/token/PagesERC721.sol /// @audit 0x01ffc9a7 164: interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 /// @audit 0x80ac58cd 165: interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721 /// @audit 0x5b5e139f 166: interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata
File: script/deploy/DeployBase.s.sol /// @audit 4 66: address gobblerAddress = LibRLP.computeAddress(tx.origin, vm.getNonce(tx.origin) + 4); /// @audit 5 67: address pageAddress = LibRLP.computeAddress(tx.origin, vm.getNonce(tx.origin) + 5);
File: src/Pages.sol /// @audit 4.2069e18 168: 4.2069e18, // Target price. /// @audit 0.31e18 169: 0.31e18, // Price decay percent. /// @audit 9000e18 170: 9000e18, // Logistic asymptote. /// @audit 0.014e18 171: 0.014e18, // Logistic time scale. /// @audit 9e18 174: 9e18 // Pages to target per day.
File: script/deploy/DeployRinkeby.s.sol /// @audit 0xb3dCcb4Cf7a26f6cf6B120Cf5A73875B7BBc655B 26: address(0xb3dCcb4Cf7a26f6cf6B120Cf5A73875B7BBc655B), /// @audit 0x01BE23585060835E02B77ef475b0Cc51aA1e0709 28: address(0x01BE23585060835E02B77ef475b0Cc51aA1e0709), /// @audit 0x2ed0feb3e7fd2022120aa84fab1945545a9f2ffc9076fd6156fa96eaff4c1311 30: 0x2ed0feb3e7fd2022120aa84fab1945545a9f2ffc9076fd6156fa96eaff4c1311, /// @audit 0.1e18 32: 0.1e18,
File: src/Goo.sol /// @audit 18 58: contract Goo is ERC20("Goo", "GOO", 18) {
File: lib/VRGDAs/src/LogisticVRGDA.sol /// @audit 1e18 44: logisticLimit = _maxSellable + 1e18; /// @audit 2e18 47: logisticLimitDoubled = logisticLimit * 2e18; /// @audit 1e18 62: return -unsafeWadDiv(wadLn(unsafeDiv(logisticLimitDoubled, sold + logisticLimit) - 1e18), timeScale);
File: lib/solmate/src/utils/LibString.sol /// @audit 0x40 13: let newFreeMemoryPointer := add(mload(0x40), 160) /// @audit 0x40 16: mstore(0x40, newFreeMemoryPointer)
File: lib/VRGDAs/src/VRGDA.sol /// @audit 1e18 29: decayConstant = wadLn(1e18 - _priceDecayPercent);
File: lib/goo-issuance/src/LibGOO.sol /// @audit 1e18 38: (emissionMultiple * lastBalanceWad * 1e18).sqrt()
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 2 instances of this issue:
File: src/Pages.sol 2: pragma solidity >=0.8.0;
File: lib/goo-issuance/src/LibGOO.sol 2: pragma solidity >=0.8.0;
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There are 2 instances of this issue:
File: src/ArtGobblers.sol 2: pragma solidity >=0.8.0;
File: script/deploy/DeployRinkeby.s.sol 2: pragma solidity >=0.8.0;
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 8 instances of this issue:
File: src/Pages.sol /// @audit seen in src/ArtGobblers.sol 86: Goo public immutable goo; /// @audit seen in src/ArtGobblers.sol 89: address public immutable community; /// @audit seen in src/ArtGobblers.sol 103: uint256 public immutable mintStart;
File: src/utils/rand/ChainlinkV1RandProvider.sol /// @audit seen in src/utils/token/PagesERC721.sol 20: ArtGobblers public immutable artGobblers;
File: script/deploy/DeployRinkeby.s.sol /// @audit seen in src/Pages.sol 11: uint256 public immutable mintStart = 1656369768;
File: src/Goo.sol /// @audit seen in src/utils/rand/ChainlinkV1RandProvider.sol 64: address public immutable artGobblers; /// @audit seen in src/ArtGobblers.sol 67: address public immutable pages;
File: src/utils/GobblerReserve.sol /// @audit seen in src/Goo.sol 18: ArtGobblers public immutable artGobblers;
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 3 instances of this issue:
File: src/ArtGobblers.sol 136: string public UNREVEALED_URI; 139: string public BASE_URI;
File: src/Pages.sol 96: string public BASE_URI;
There are 2 instances of this issue:
File: script/deploy/DeployBase.s.sol
File: script/deploy/DeployRinkeby.s.sol
There are 10 instances of this issue:
File: src/ArtGobblers.sol /// @audit Missing: '@param _pages' 278 /// @notice Sets VRGDA parameters, mint config, relevant addresses, and URIs. 279 /// @param _merkleRoot Merkle root of mint mintlist. 280 /// @param _mintStart Timestamp for the start of the VRGDA mint. 281 /// @param _goo Address of the Goo contract. 282 /// @param _team Address of the team reserve. 283 /// @param _community Address of the community reserve. 284 /// @param _randProvider Address of the randomness provider. 285 /// @param _baseUri Base URI for revealed gobblers. 286 /// @param _unrevealedUri URI for unrevealed gobblers. 287 constructor( 288 // Mint config: 289 bytes32 _merkleRoot, 290 uint256 _mintStart, 291 // Addresses: 292 Goo _goo, 293 Pages _pages, 294 address _team, 295 address _community, 296 RandProvider _randProvider, 297 // URIs: 298 string memory _baseUri, 299 string memory _unrevealedUri 300 ) 301 GobblersERC721("Art Gobblers", "GOBBLER") 302 Owned(msg.sender) 303 LogisticVRGDA( 304 69.42e18, // Target price. 305 0.31e18, // Price decay percent. 306 // Max gobblers mintable via VRGDA. 307 toWadUnsafe(MAX_MINTABLE), 308 0.0023e18 // Time scale. 309: ) /// @audit Missing: '@param bytes32' 544 /// @notice Callback from rand provider. Sets randomSeed. Can only be called by the rand provider. 545 /// @param randomness The 256 bits of verifiable randomness provided by the rand provider. 546: function acceptRandomSeed(bytes32, uint256 randomness) external { /// @audit Missing: '@return' 691 /// @notice Returns a token's URI if it has been minted. 692 /// @param gobblerId The id of the token to get the URI for. 693: function tokenURI(uint256 gobblerId) public view virtual override returns (string memory) { /// @audit Missing: '@return' 755 /// @notice Calculate a user's virtual goo balance. 756 /// @param user The user to query balance for. 757: function gooBalance(address user) public view returns (uint256) { /// @audit Missing: '@return' 837 /// @dev Gobblers minted to reserves cannot comprise more than 20% of the sum of 838 /// the supply of goo minted gobblers and the supply of gobblers minted to reserves. 839: function mintReservedGobblers(uint256 numGobblersEach) external returns (uint256 lastMintedGobblerId) { /// @audit Missing: '@return' 864 /// @notice Convenience function to get emissionMultiple for a gobbler. 865 /// @param gobblerId The gobbler to get emissionMultiple for. 866: function getGobblerEmissionMultiple(uint256 gobblerId) external view returns (uint256) { /// @audit Missing: '@return' 870 /// @notice Convenience function to get emissionMultiple for a user. 871 /// @param user The user to get emissionMultiple for. 872: function getUserEmissionMultiple(address user) external view returns (uint256) {
File: src/Pages.sol /// @audit Missing: '@return' 237 /// @dev Pages minted to the reserve cannot comprise more than 10% of the sum of the 238 /// supply of goo minted pages and the supply of pages minted to the community reserve. 239: function mintCommunityPages(uint256 numPages) external returns (uint256 lastMintedPageId) { /// @audit Missing: '@return' 263 /// @notice Returns a page's URI if it has been minted. 264 /// @param pageId The id of the page to get the URI for. 265: function tokenURI(uint256 pageId) public view virtual override returns (string memory) {
File: lib/goo-issuance/src/LibGOO.sol /// @audit Missing: '@return' 15 /// @param lastBalanceWad The last checkpointed balance to apply the emission multiple over time to, scaled by 1e18. 16 /// @param timeElapsedWad The time elapsed since the last checkpoint, scaled by 1e18. 17 function computeGOOBalance( 18 uint256 emissionMultiple, 19 uint256 lastBalanceWad, 20 uint256 timeElapsedWad 21: ) public pure returns (uint256) {
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 16 instances of this issue:
File: src/ArtGobblers.sol 229: event GooBalanceUpdated(address indexed user, uint256 newGooBalance); 232: event GobblerPurchased(address indexed user, uint256 indexed gobblerId, uint256 price); 233: event LegendaryGobblerMinted(address indexed user, uint256 indexed gobblerId, uint256[] burnedGobblerIds); 234: event ReservedGobblersMinted(address indexed user, uint256 lastMintedGobblerId, uint256 numGobblersEach); 236: event RandomnessFulfilled(uint256 randomness); 237: event RandomnessRequested(address indexed user, uint256 toBeRevealed); 240: event GobblersRevealed(address indexed user, uint256 numGobblers, uint256 lastRevealedId);
File: lib/solmate/src/tokens/ERC721.sol 15: event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
File: src/utils/token/GobblersERC1155B.sol 29: event ApprovalForAll(address indexed owner, address indexed operator, bool approved); 31: event URI(string value, uint256 indexed id);
File: src/utils/token/GobblersERC721.sol 17: event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
File: src/utils/token/PagesERC721.sol 18: event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
File: src/Pages.sol 134: event PagePurchased(address indexed user, uint256 indexed pageId, uint256 price); 136: event CommunityPagesMinted(address indexed user, uint256 lastMintedPageId, uint256 numPages);
File: src/utils/rand/RandProvider.sol 13: event RandomBytesRequested(bytes32 requestId); 14: event RandomBytesReturned(bytes32 requestId, uint256 randomness);
Consider changing the variable to be an unnamed one
There are 3 instances of this issue:
File: src/utils/token/GobblersERC1155B.sol /// @audit owner 55: function ownerOf(uint256 id) public view virtual returns (address owner) {
File: src/utils/token/PagesERC721.sol /// @audit isApproved 72: function isApprovedForAll(address owner, address operator) public view returns (bool isApproved) {
File: src/utils/rand/ChainlinkV1RandProvider.sol /// @audit requestId 62: function requestRandomBytes() external returns (bytes32 requestId) {
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 2 instances of this issue:
File: src/ArtGobblers.sol 705: if (gobblerId < FIRST_LEGENDARY_GOBBLER_ID) revert("NOT_MINTED");
File: lib/solmate/src/tokens/ERC721.sol 158: require(to != address(0), "INVALID_RECIPIENT");
#0 - GalloDaSballo
2022-10-14T00:51:55Z
[Lā01] Don't roll your own crypto R for this specific case as I'm not convinced the entropy to be misused, recommend following up with sponsor if you find further info
[Lā02] Chainlink's VRF V1 is deprecated R as system enables swapping [Lā03] Rinkeby is not supported for Chainlink's VRF R, nice find
[Lā04] Missing checks for address(0x0) when assigning values to address state variables L
[Nā01] requestId is always zero NC
[Nā02] Don't use periods with fragments I don't have an opinion about this one
[Nā03] Contract implements interface without extending the interface R
[Nā04] public functions not called by the contract should be declared external instead R [Nā05] constants should be defined rather than using magic numbers R
[Nā06] Use a more recent version of solidity R [Nā08] Constant redefined elsewhere This one looks off as those are immutable and not known until deploy time
[Nā09] Variable names that consist of all capital letters should be reserved for constant/immutable variables R
[Nā10] File is missing NatSpec NC [Nā11] NatSpec is incomplete NC [Nā12] Event is missing indexed fields I still don't get why you'd index "gibberish" values if it doesn't even save gas
[Nā13] Not using the named return variables anywhere in the function is confusing R
[Nā14] Duplicated require()/revert() checks should be refactored to a modifier or function R
Overall pretty good report with some interesting ideas, of the automated ones def the best
š Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xSmartContract, Atarpara, CertoraInc, Deathstore, Deivitto, ElKu, MiloTruck, ReyAdmirado, SnowMan, Tadashi, V_B, __141345__, aviggiano, catchup, djxploit, gogo, pfapostol, philogy, shung
1137.9873 USDC - $1,137.99
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Save gas by not updating seed | 1 | 2897 |
| [Gā02] | State variables only set in the constructor should be declared immutable | 7 | 12582 |
| [Gā03] | Avoid contract existence checks by using solidity version 0.8.10 or later | 11 | 1100 |
| [Gā04] | State variables should be cached in stack variables rather than re-reading them from storage | 4 | 388 |
| [Gā05] | Multiple accesses of a mapping/array should use a local variable cache | 25 | 1050 |
| [Gā06] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 2 | 226 |
| [Gā07] | <array>.length should not be looked up in every loop of a for-loop | 2 | 6 |
| [Gā08] | Optimize names to save gas | 10 | 220 |
| [Gā09] | Using bools for storage incurs overhead | 5 | 68400 |
| [Gā10] | Use a more recent version of solidity | 22 | - |
| [Gā11] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 9 | 45 |
| [Gā12] | Using private rather than public for constants, saves gas | 18 | - |
| [Gā13] | Division by two should use bit shifting | 1 | 20 |
| [Gā14] | Stack variable used as a cheaper cache for a state variable is only used once | 2 | 6 |
| [Gā15] | Use custom errors rather than revert()/require() strings to save gas | 36 | - |
| [Gā16] | Functions guaranteed to revert when called by normal users can be marked payable | 3 | 63 |
Total: 158 instances over 16 issues with 87003 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions
The code that determines a set of random values based on the seed does not follow Chainlink's stated best practice for doing so. By updating the seed rather than including a counter in what's hashed (e.g. currentId), the code incurs an unnecessary Gsreset 2900 gas.
There is 1 instance of this issue:
File: /src/ArtGobblers.sol 664 // Update the random seed to choose a new distance for the next iteration. 665 // It is critical that we cast to uint64 here, as otherwise the random seed 666 // set after calling revealGobblers(1) thrice would differ from the seed set 667 // after calling revealGobblers(3) a single time. This would enable an attacker 668 // to choose from a number of different seeds and use whichever is most favorable. 669 // Equivalent to randomSeed = uint64(uint256(keccak256(abi.encodePacked(randomSeed)))) 670 assembly { 671 mstore(0, randomSeed) // Store the random seed in scratch space. 672 673 // Moduloing by 1 << 64 (2 ** 64) is equivalent to a uint64 cast. 674 randomSeed := mod(keccak256(0, 32), shl(64, 1)) 675: }
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
While strings are not value types, and therefore cannot be immutable/constant if not hard-coded outside of the constructor, the same behavior can be achieved by making the current contract abstract with virtual functions for the string accessors, and having a child contract override the functions with the hard-coded implementation-specific values.
There are 7 instances of this issue:
File: src/ArtGobblers.sol /// @audit UNREVEALED_URI (constructor) 321: UNREVEALED_URI = _unrevealedUri; /// @audit UNREVEALED_URI (access) 702: if (gobblerId <= currentNonLegendaryId) return UNREVEALED_URI; /// @audit BASE_URI (constructor) 320: BASE_URI = _baseUri; /// @audit BASE_URI (access) 698: return string.concat(BASE_URI, uint256(getGobblerData[gobblerId].idx).toString()); /// @audit BASE_URI (access) 709: return string.concat(BASE_URI, gobblerId.toString());
File: src/Pages.sol /// @audit BASE_URI (constructor) 183: BASE_URI = _baseUri; /// @audit BASE_URI (access) 268: return string.concat(BASE_URI, pageId.toString());
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 11 instances of this issue:
File: src/ArtGobblers.sol /// @audit toString() 698: return string.concat(BASE_URI, uint256(getGobblerData[gobblerId].idx).toString());
File: lib/solmate/src/tokens/ERC721.sol /// @audit onERC721Received() 120: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == /// @audit onERC721Received() 136: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) == /// @audit onERC721Received() 198: ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") == /// @audit onERC721Received() 213: ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, data) ==
File: src/utils/token/GobblersERC1155B.sol /// @audit onERC1155Received() 150: ERC1155TokenReceiver(to).onERC1155Received(msg.sender, address(0), id, 1, data) == /// @audit onERC1155BatchReceived() 186: ERC1155TokenReceiver(to).onERC1155BatchReceived(msg.sender, address(0), ids, amounts, data) ==
File: src/utils/token/GobblersERC721.sol /// @audit onERC721Received() 123: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == /// @audit onERC721Received() 139: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
File: src/utils/token/PagesERC721.sol /// @audit onERC721Received() 136: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == /// @audit onERC721Received() 152: ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 4 instances of this issue:
File: src/ArtGobblers.sol /// @audit BASE_URI on line 698 709: return string.concat(BASE_URI, gobblerId.toString()); /// @audit numMintedFromGoo on line 482 493: uint256 numMintedSinceStart = numMintedFromGoo - numMintedAtStart; /// @audit getGobblerData[swapId].idx on line 615 617: : getGobblerData[swapId].idx; // Shuffled before. /// @audit getGobblerData[currentId].idx on line 623 625: : getGobblerData[currentId].idx; // Shuffled before.
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata
There are 25 instances of this issue:
File: src/ArtGobblers.sol /// @audit getGobblerData[id] on line 437 439: burnedMultipleTotal += getGobblerData[id].emissionMultiple; /// @audit getGobblerData[id] on line 439 441: emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id); /// @audit getUserData[<etc>] on line 454 455: getUserData[msg.sender].lastTimestamp = uint64(block.timestamp); // Store time alongside it. /// @audit getUserData[<etc>] on line 455 456: getUserData[msg.sender].emissionMultiple += uint32(burnedMultipleTotal); // Update multiple. /// @audit getUserData[<etc>] on line 456 /// @audit getUserData[<etc>] on line 458 458: getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost + 1); /// @audit getGobblerData[swapId] on line 615 617: : getGobblerData[swapId].idx; // Shuffled before. /// @audit getGobblerData[currentId] on line 620 623: uint64 currentIndex = getGobblerData[currentId].idx == 0 /// @audit getGobblerData[currentId] on line 623 625: : getGobblerData[currentId].idx; // Shuffled before. /// @audit getGobblerData[currentId] on line 625 649: getGobblerData[currentId].idx = swapIndex; /// @audit getGobblerData[currentId] on line 649 650: getGobblerData[currentId].emissionMultiple = uint32(newCurrentIdMultiple); /// @audit getGobblerData[swapId] on line 617 653: getGobblerData[swapId].idx = currentIndex; /// @audit getUserData[currentIdOwner] on line 660 661: getUserData[currentIdOwner].lastTimestamp = uint64(block.timestamp); /// @audit getUserData[currentIdOwner] on line 661 662: getUserData[currentIdOwner].emissionMultiple += uint32(newCurrentIdMultiple); /// @audit getUserData[user] on line 761 762: getUserData[user].lastBalance, /// @audit getUserData[user] on line 762 763: uint(toDaysWadUnsafe(block.timestamp - getUserData[user].lastTimestamp)) /// @audit getUserData[user] on line 825 826: getUserData[user].lastTimestamp = uint64(block.timestamp); /// @audit getGobblerData[id] on line 885 896: getGobblerData[id].owner = to; /// @audit getGobblerData[id] on line 896 899: uint32 emissionMultiple = getGobblerData[id].emissionMultiple; // Caching saves gas. /// @audit getUserData[from] on line 903 904: getUserData[from].lastTimestamp = uint64(block.timestamp); /// @audit getUserData[from] on line 904 905: getUserData[from].emissionMultiple -= emissionMultiple; /// @audit getUserData[from] on line 905 906: getUserData[from].gobblersOwned -= 1; /// @audit getUserData[to] on line 910 911: getUserData[to].lastTimestamp = uint64(block.timestamp); /// @audit getUserData[to] on line 911 912: getUserData[to].emissionMultiple += emissionMultiple; /// @audit getUserData[to] on line 912 913: getUserData[to].gobblersOwned += 1;
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 2 instances of this issue:
File: src/ArtGobblers.sol 844: uint256 newNumMintedForReserves = numMintedForReserves += (numGobblersEach << 1);
File: src/Pages.sol 244: uint256 newNumMintedForCommunity = numMintedForCommunity += uint128(numPages);
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 2 instances of this issue:
File: src/utils/token/GobblersERC1155B.sol 114: for (uint256 i = 0; i < owners.length; ++i) {
File: src/utils/GobblerReserve.sol 37: for (uint256 i = 0; i < ids.length; i++) {
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 10 instances of this issue:
File: src/ArtGobblers.sol /// @audit claimGobbler(), mintFromGoo(), gobblerPrice(), mintLegendaryGobbler(), legendaryGobblerPrice(), requestRandomSeed(), acceptRandomSeed(), upgradeRandProvider(), revealGobblers(), gobble(), gooBalance(), addGoo(), removeGoo(), burnGooForPages(), mintReservedGobblers(), getGobblerEmissionMultiple(), getUserEmissionMultiple() 83: contract ArtGobblers is GobblersERC721, LogisticVRGDA, Owned, ERC1155TokenReceiver {
File: script/deploy/DeployBase.s.sol /// @audit run() 16: abstract contract DeployBase is Script {
File: src/Pages.sol /// @audit mintFromGoo(), pagePrice(), mintCommunityPages() 78: contract Pages is PagesERC721, LogisticToLinearVRGDA {
File: src/utils/rand/ChainlinkV1RandProvider.sol /// @audit requestRandomBytes() 14: contract ChainlinkV1RandProvider is RandProvider, VRFConsumerBase {
File: src/Goo.sol /// @audit mintForGobblers(), burnForGobblers(), burnForPages() 58: contract Goo is ERC20("Goo", "GOO", 18) {
File: lib/VRGDAs/src/VRGDA.sol /// @audit getVRGDAPrice(), getTargetSaleTime() 10: abstract contract VRGDA {
File: lib/goo-issuance/src/LibGOO.sol /// @audit computeGOOBalance() 10: library LibGOO {
File: lib/solmate/src/auth/Owned.sol /// @audit setOwner() 6: abstract contract Owned {
File: src/utils/GobblerReserve.sol /// @audit withdraw() 12: contract GobblerReserve is Owned {
File: src/utils/rand/RandProvider.sol /// @audit requestRandomBytes() 8: interface RandProvider {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 5 instances of this issue:
File: src/ArtGobblers.sol 149: mapping(address => bool) public hasClaimedMintlistGobbler;
File: lib/solmate/src/tokens/ERC721.sol 51: mapping(address => mapping(address => bool)) public isApprovedForAll;
File: src/utils/token/GobblersERC1155B.sol 37: mapping(address => mapping(address => bool)) public isApprovedForAll;
File: src/utils/token/GobblersERC721.sol 77: mapping(address => mapping(address => bool)) public isApprovedForAll;
File: src/utils/token/PagesERC721.sol 70: mapping(address => mapping(address => bool)) internal _isApprovedForAll;
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 22 instances of this issue:
File: src/ArtGobblers.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/utils/FixedPointMathLib.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/tokens/ERC721.sol 2: pragma solidity >=0.8.0;
File: src/utils/token/GobblersERC1155B.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/utils/SignedWadMath.sol 2: pragma solidity >=0.8.0;
File: src/utils/token/GobblersERC721.sol 2: pragma solidity >=0.8.0;
File: src/utils/token/PagesERC721.sol 2: pragma solidity >=0.8.0;
File: script/deploy/DeployBase.s.sol 2: pragma solidity >=0.8.0;
File: src/Pages.sol 2: pragma solidity >=0.8.0;
File: src/utils/rand/ChainlinkV1RandProvider.sol 2: pragma solidity >=0.8.0;
File: lib/VRGDAs/src/LogisticToLinearVRGDA.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/utils/MerkleProofLib.sol 2: pragma solidity >=0.8.0;
File: script/deploy/DeployRinkeby.s.sol 2: pragma solidity >=0.8.0;
File: src/Goo.sol 2: pragma solidity >=0.8.0;
File: lib/VRGDAs/src/LogisticVRGDA.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/utils/LibString.sol 2: pragma solidity >=0.8.0;
File: lib/VRGDAs/src/VRGDA.sol 2: pragma solidity >=0.8.0;
File: lib/goo-issuance/src/LibGOO.sol 2: pragma solidity >=0.8.0;
File: lib/solmate/src/auth/Owned.sol 2: pragma solidity >=0.8.0;
File: lib/VRGDAs/src/LinearVRGDA.sol 2: pragma solidity >=0.8.0;
File: src/utils/GobblerReserve.sol 2: pragma solidity >=0.8.0;
File: src/utils/rand/RandProvider.sol 2: pragma solidity >=0.8.0;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 9 instances of this issue:
File: lib/solmate/src/tokens/ERC721.sol 99: _balanceOf[from]--; 101: _balanceOf[to]++; 164: _balanceOf[to]++; 179: _balanceOf[owner]--;
File: src/utils/token/PagesERC721.sol 115: _balanceOf[from]--; 117: _balanceOf[to]++; 181: _balanceOf[to]++;
File: src/Pages.sol 251: for (uint256 i = 0; i < numPages; i++) _mint(community, ++lastMintedPageId);
File: src/utils/GobblerReserve.sol 37: for (uint256 i = 0; i < ids.length; i++) {
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 18 instances of this issue:
File: src/ArtGobblers.sol 112: uint256 public constant MAX_SUPPLY = 10000; 115: uint256 public constant MINTLIST_SUPPLY = 2000; 118: uint256 public constant LEGENDARY_SUPPLY = 10; 122: uint256 public constant RESERVED_SUPPLY = (MAX_SUPPLY - MINTLIST_SUPPLY - LEGENDARY_SUPPLY) / 5; 126 uint256 public constant MAX_MINTABLE = MAX_SUPPLY 127 - MINTLIST_SUPPLY 128 - LEGENDARY_SUPPLY 129: - RESERVED_SUPPLY; 146: bytes32 public immutable merkleRoot; 156: uint256 public immutable mintStart; 177: uint256 public constant LEGENDARY_GOBBLER_INITIAL_START_PRICE = 69; 180: uint256 public constant FIRST_LEGENDARY_GOBBLER_ID = MAX_SUPPLY - LEGENDARY_SUPPLY + 1; 184: uint256 public constant LEGENDARY_AUCTION_INTERVAL = MAX_MINTABLE / (LEGENDARY_SUPPLY + 1);
File: src/Pages.sol 103: uint256 public immutable mintStart;
File: script/deploy/DeployRinkeby.s.sol 11: uint256 public immutable mintStart = 1656369768; 13: string public constant gobblerBaseUri = "https://testnet.ag.xyz/api/nfts/gobblers/"; 14: string public constant gobblerUnrevealedUri = "https://testnet.ag.xyz/api/nfts/unrevealed"; 15: string public constant pagesBaseUri = "https://testnet.ag.xyz/api/nfts/pages/";
File: lib/VRGDAs/src/LogisticVRGDA.sol 20: int256 public immutable logisticLimit; 25: int256 public immutable logisticLimitDoubled;
File: lib/VRGDAs/src/VRGDA.sol 17: int256 public immutable targetPrice;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There is 1 instance of this issue:
File: src/ArtGobblers.sol 462: cost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE / 2 ? LEGENDARY_GOBBLER_INITIAL_START_PRICE : cost * 2
If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend
There are 2 instances of this issue:
File: src/ArtGobblers.sol 480: uint256 startPrice = legendaryGobblerAuctionData.startPrice; 481: uint256 numSold = legendaryGobblerAuctionData.numSold;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 36 instances of this issue:
File: src/ArtGobblers.sol 437: require(getGobblerData[id].owner == msg.sender, "WRONG_FROM"); 885: require(from == getGobblerData[id].owner, "WRONG_FROM"); 887: require(to != address(0), "INVALID_RECIPIENT"); 889 require( 890 msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id], 891 "NOT_AUTHORIZED" 892: );
File: lib/solmate/src/tokens/ERC721.sol 36: require((owner = _ownerOf[id]) != address(0), "NOT_MINTED"); 40: require(owner != address(0), "ZERO_ADDRESS"); 69: require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "NOT_AUTHORIZED"); 87: require(from == _ownerOf[id], "WRONG_FROM"); 89: require(to != address(0), "INVALID_RECIPIENT"); 91 require( 92 msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id], 93 "NOT_AUTHORIZED" 94: ); 118 require( 119 to.code.length == 0 || 120 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == 121 ERC721TokenReceiver.onERC721Received.selector, 122 "UNSAFE_RECIPIENT" 123: ); 134 require( 135 to.code.length == 0 || 136 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) == 137 ERC721TokenReceiver.onERC721Received.selector, 138 "UNSAFE_RECIPIENT" 139: ); 158: require(to != address(0), "INVALID_RECIPIENT"); 160: require(_ownerOf[id] == address(0), "ALREADY_MINTED"); 175: require(owner != address(0), "NOT_MINTED"); 196 require( 197 to.code.length == 0 || 198 ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") == 199 ERC721TokenReceiver.onERC721Received.selector, 200 "UNSAFE_RECIPIENT" 201: ); 211 require( 212 to.code.length == 0 || 213 ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, data) == 214 ERC721TokenReceiver.onERC721Received.selector, 215 "UNSAFE_RECIPIENT" 216: );
File: src/utils/token/GobblersERC1155B.sol 107: require(owners.length == ids.length, "LENGTH_MISMATCH"); 149 require( 150 ERC1155TokenReceiver(to).onERC1155Received(msg.sender, address(0), id, 1, data) == 151 ERC1155TokenReceiver.onERC1155Received.selector, 152 "UNSAFE_RECIPIENT" 153: ); 185 require( 186 ERC1155TokenReceiver(to).onERC1155BatchReceived(msg.sender, address(0), ids, amounts, data) == 187 ERC1155TokenReceiver.onERC1155BatchReceived.selector, 188 "UNSAFE_RECIPIENT" 189: );
File: lib/solmate/src/utils/SignedWadMath.sol 142: require(x > 0, "UNDEFINED");
File: src/utils/token/GobblersERC721.sol 62: require((owner = getGobblerData[id].owner) != address(0), "NOT_MINTED"); 66: require(owner != address(0), "ZERO_ADDRESS"); 95: require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "NOT_AUTHORIZED"); 121 require( 122 to.code.length == 0 || 123 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == 124 ERC721TokenReceiver.onERC721Received.selector, 125 "UNSAFE_RECIPIENT" 126: ); 137 require( 138 to.code.length == 0 || 139 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) == 140 ERC721TokenReceiver.onERC721Received.selector, 141 "UNSAFE_RECIPIENT" 142: );
File: src/utils/token/PagesERC721.sol 55: require((owner = _ownerOf[id]) != address(0), "NOT_MINTED"); 59: require(owner != address(0), "ZERO_ADDRESS"); 85: require(msg.sender == owner || isApprovedForAll(owner, msg.sender), "NOT_AUTHORIZED"); 103: require(from == _ownerOf[id], "WRONG_FROM"); 105: require(to != address(0), "INVALID_RECIPIENT"); 107 require( 108 msg.sender == from || isApprovedForAll(from, msg.sender) || msg.sender == getApproved[id], 109 "NOT_AUTHORIZED" 110: ); 135 require( 136 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == 137 ERC721TokenReceiver.onERC721Received.selector, 138 "UNSAFE_RECIPIENT" 139: ); 151 require( 152 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) == 153 ERC721TokenReceiver.onERC721Received.selector, 154 "UNSAFE_RECIPIENT" 155: );
File: lib/VRGDAs/src/VRGDA.sol 32: require(decayConstant < 0, "NON_NEGATIVE_DECAY_CONSTANT");
File: lib/solmate/src/auth/Owned.sol 20: require(msg.sender == owner, "UNAUTHORIZED");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 3 instances of this issue:
File: src/ArtGobblers.sol 560: function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
File: lib/solmate/src/auth/Owned.sol 39: function setOwner(address newOwner) public virtual onlyOwner {
File: src/utils/GobblerReserve.sol 34: function withdraw(address to, uint256[] calldata ids) external onlyOwner {
#0 - GalloDaSballo
2022-10-05T22:44:33Z
I'm not sure what this finding implies as if we were to re-use the same randomness input and then use a iterator, we'd need to store a new uint of the values we've revealed, incurring still a 5k gas cost for setting that (as we may want to re-use the same randomness value until exhausted)
I'll give 100 gas, would ask to send a coded test next time to get a better score
6.3k for the three URI variables
Will accept because explained, but will cap at 500 500
300 gas ignoring the URI as it's in 2 separate mutually exclusive returns
Will give 500 in lack of benchmark (Foundry Output), mostly valid but unsure of the effective savings
Rest will give you a ballpark 150 gas saved, better than the average report by presentation and detail, however those are not as high impact as the ones above
Overall one of the best reports, I think adding custom benchmarks would make it the best by far
7850