Platform: Code4rena
Start Date: 03/05/2022
Pot Size: $30,000 USDC
Total HM: 6
Participants: 93
Period: 3 days
Judge: gzeon
Id: 118
League: ETH
Rank: 3/93
Findings: 5
Award: $1,878.54
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: VAD37
Also found by: AuditsAreUS, IllIllI, MaratCerby, rfa, sorrynotsorry
1116.8018 USDC - $1,116.80
Judge has assessed an item in Issue #47 as Medium risk. The relevant finding follows:
transfer()/transferFrom() with IERC20Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 175 token.transfer(msg.sender, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 402 IERC20(weth).transfer(to, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 629 token.transfer(msg.sender, amount);
#0 - gzeoneth
2022-06-18T17:09:02Z
Duplicate of #70
🌟 Selected for report: pedroais
Also found by: AuditsAreUS, BowTiedWardens, GimelSec, IllIllI, WatchPug, defsec, leastwood
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L35-L36 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L141-L147
The purpose of a provenance hash is to help ensure that the auction is fair - that the sellers aren't able to deterministically assign who gets what, after all of the money has been spent, and that the buyers don't know which NFTs to focus their energies on buying. The ability to set it after funds have been transferred, effectively allows the seller to steal from the buyers in an undetectable way: once the seller knows the IDs of the NFTs he/she has bought via various sybil accounts, the seller can generate a provenance hash that favors the NFTs in those accounts, leaving the less valuable ones for the other buyers. It is important for the hash to be set in stone before money changes hands.
The auction that buyers thought was fair was in fact unfair, and because of this, buyers spend more to acquire their NFTs than they otherwise would have chosen to had they known it was unfair.
The hash is not a constant nor is it immutable
File: contracts/ForgottenRunesWarriorsGuild.sol #1 35 /// @notice The provenance hash 36 string public METADATA_PROVENANCE_HASH = '';
The hash can be changed at any time. The team running the auction can 'forget' to set it during the auction, and once they determine which NFTs they now control, they can set the hash to favor their NFT holdings, choosing some of the most valuable ones for themselves. It need not be after the whole auction is done - it can be set once they've bought a sufficient number of NFTs.
File: contracts/ForgottenRunesWarriorsGuild.sol #2 141 /** 142 * @dev Sets provenance hash 143 * @param newHash string of the new minter 144 */ 145 function setProvenanceHash(string memory newHash) public onlyOwner { 146 METADATA_PROVENANCE_HASH = newHash; 147 }
Code inspection
Ideally the METADATA_PROVENANCE_HASH would be immutable and set in the constructor. At the very least setProvenanceHash() should revert after the first user mints, and mint() should revert if the hash has not yet been set.
#0 - wagmiwiz
2022-05-05T10:17:38Z
acknowledged but won't change
#1 - KenzoAgada
2022-06-06T07:08:50Z
Duplicate of #49.
#2 - gzeoneth
2022-06-18T18:15:38Z
Duplicate of #38
48.5872 USDC - $48.59
Judge has assessed an item in Issue #47 as Medium risk. The relevant finding follows:
payable.transfer()/payable.send()The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:
payable callbackpayable callback spends more than 2300 gas (which is only enough to emit something)Address.sendValue() insteadFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 164 require(payable(msg.sender).send(address(this).balance));
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 610 require(payable(vault).send(_amount));
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 618 require(payable(vault).send(address(this).balance));
#0 - gzeoneth
2022-06-18T19:19:25Z
Duplicate of #254
🌟 Selected for report: defsec
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0x52, 0xDjango, 0xf15ers, 0xkatana, 0xliumin, AuditsAreUS, BowTiedWardens, CertoraInc, Cr4ckM3, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, Picodes, Ruhum, TerrierLover, TrungOre, VAD37, WatchPug, berndartmueller, broccolirob, catchup, cccz, cryptphi, csanuragjain, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, hubble, hyh, ilan, joestakey, kebabsec, kenta, kenzo, leastwood, m9800, marximimus, minhquanym, oyc_109, p4st13r4, pauliax, pedroais, peritoflores, plotchy, rajatbeladiya, reassor, rfa, robee, rotcivegaf, samruna, shenwilly, shung, simon135, sorrynotsorry, sseefried, teddav, throttle, tintin, unforgiven, z3s
149.3609 USDC - $149.36
transfer()/transferFrom() with IERC20Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 175 token.transfer(msg.sender, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 402 IERC20(weth).transfer(to, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 629 token.transfer(msg.sender, amount);
transfer()/transferFrom() not checkedNot all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 175 token.transfer(msg.sender, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 402 IERC20(weth).transfer(to, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 629 token.transfer(msg.sender, amount);
address(0x0) when assigning values to address state variablesFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 138 minter = newMinter;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 528 vault = _newVaultAddress;
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 544 weth = _newWethAddress;
payable.transfer()/payable.send()The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:
payable callbackpayable callback spends more than 2300 gas (which is only enough to emit something)Address.sendValue() insteadFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 164 require(payable(msg.sender).send(address(this).balance));
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 610 require(payable(vault).send(_amount));
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 618 require(payable(vault).send(address(this).balance));
selfRefundsStartTime not updated when the other values are updatedsetPhaseTimes() does not have a variable for updating the selfRefundsStartTime variable, so it may be overlooked. After watching your explanation video, it seems like a better approach would be to have selfRefundsStartTime become selfRefundsDelay and have selfRefundsStarted() return whether the current timestamp is past publicStartTime + selfRefundsDelay. That way if you "get hit by a bus", as you said, users will always be able to self-refund.
File: contracts/ForgottenRunesWarriorsMinter.sol #1 480 function setPhaseTimes( 481 uint256 newDaStartTime, 482 uint256 newMintlistStartTime, 483 uint256 newPublicStartTime, 484 uint256 newClaimsStartTime 485 ) public onlyOwner {
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 353 nonReentrant
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 364 function refundAddress(address minter) public onlyOwner nonReentrant {
require()/revert() statements should have descriptive reason stringsFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 164 require(payable(msg.sender).send(address(this).balance));
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 174 require(address(msg.sender) != address(0));
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 610 require(payable(vault).send(_amount));
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 618 require(payable(vault).send(address(this).balance));
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 628 require(address(msg.sender) != address(0));
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.
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 52 function initialize(address newMinter) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 85 function exists(uint256 tokenId) public view returns (bool) {
File: /contracts/ForgottenRunesWarriorsGuild.sol #3 145 function setProvenanceHash(string memory newHash) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #4 163 function withdrawAll() public payable onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #5 173 function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #6 337 function numDaMinters() public view returns (uint256) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #7 350 function issueRefunds(uint256 startIdx, uint256 endIdx) 351 public 352 onlyOwner 353 nonReentrant
File: /contracts/ForgottenRunesWarriorsMinter.sol #8 364 function refundAddress(address minter) public onlyOwner nonReentrant {
File: /contracts/ForgottenRunesWarriorsMinter.sol #9 371 function selfRefund() public nonReentrant {
File: /contracts/ForgottenRunesWarriorsMinter.sol #10 427 function pause() public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #11 434 function unpause() public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #12 469 function setSelfRefundsStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #13 480 function setPhaseTimes( 481 uint256 newDaStartTime, 482 uint256 newMintlistStartTime, 483 uint256 newPublicStartTime, 484 uint256 newClaimsStartTime 485 ) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #14 505 function setMintlist1MerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #15 513 function setMintlist2MerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #16 520 function setClaimlistMerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #17 550 function setStartPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #18 557 function setLowestPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #19 564 function setDaPriceCurveLength(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #20 571 function setDaDropInterval(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #21 579 function setFinalPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #22 586 function setMaxDaSupply(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #23 593 function setMaxForSale(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #24 600 function setMaxForClaim(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #25 608 function withdraw(uint256 _amount) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #26 616 function withdrawAll() public payable onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #27 627 function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
constants should be defined rather than using magic numbersFile: /contracts/ForgottenRunesWarriorsMinter.sol #1 141 numWarriors > 0 && numWarriors <= 20,
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 211 numWarriors > 0 && numWarriors <= 20,
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 416 (bool success, ) = to.call{value: value, gas: 30_000}(new bytes(0));
The type of the variable is the same as the type to which the variable is being cast
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 258 require(address(recipient) != address(0), 'address req');
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 609 require(address(vault) != address(0), 'no vault');
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 617 require(address(vault) != address(0), 'no vault');
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>)
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 1 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 1 pragma solidity ^0.8.0;
const/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).
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 36 string public METADATA_PROVENANCE_HASH = '';
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 1 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 1 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 0 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 0 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 106 /** 107 * @dev Create the contract and set the initial baseURI 108 * @param _warriors address the initial warriors contract address 109 */ 110 constructor(IForgottenRunesWarriorsGuild _warriors, address _weth) {
Missing: @param _weth
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3//contracts/ForgottenRunesWarriorsMinter.sol#L106-L110
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 386 * @param minter address the address of the account that wants a refund 387 */ 388 function refundOwed(address minter) public view returns (uint256) {
Missing: @return
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3//contracts/ForgottenRunesWarriorsMinter.sol#L386-L388
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 410 * @param value uint256 how much ETH to send 411 */ 412 function _safeTransferETH(address to, uint256 value) 413 internal 414 returns (bool)
Missing: @return
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3//contracts/ForgottenRunesWarriorsMinter.sol#L410-L414
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 129 function setBaseURI(string memory newBaseURI) public onlyOwner { 130 baseTokenURI = newBaseURI; 131 }
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 137 function setMinter(address newMinter) public onlyOwner { 138 minter = newMinter; 139 }
File: /contracts/ForgottenRunesWarriorsGuild.sol #3 145 function setProvenanceHash(string memory newHash) public onlyOwner { 146 METADATA_PROVENANCE_HASH = newHash; 147 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 441 function setDaStartTime(uint256 _newTime) public onlyOwner { 442 daStartTime = _newTime; 443 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 448 function setMintlistStartTime(uint256 _newTime) public onlyOwner { 449 mintlistStartTime = _newTime; 450 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #6 455 function setPublicStartTime(uint256 _newTime) public onlyOwner { 456 publicStartTime = _newTime; 457 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #7 462 function setClaimsStartTime(uint256 _newTime) public onlyOwner { 463 claimsStartTime = _newTime; 464 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #8 469 function setSelfRefundsStartTime(uint256 _newTime) public onlyOwner { 470 selfRefundsStartTime = _newTime; 471 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #9 505 function setMintlist1MerkleRoot(bytes32 newMerkleRoot) public onlyOwner { 506 mintlist1MerkleRoot = newMerkleRoot; 507 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #10 513 function setMintlist2MerkleRoot(bytes32 newMerkleRoot) public onlyOwner { 514 mintlist2MerkleRoot = newMerkleRoot; 515 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #11 520 function setClaimlistMerkleRoot(bytes32 newMerkleRoot) public onlyOwner { 521 claimlistMerkleRoot = newMerkleRoot; 522 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #12 527 function setVaultAddress(address _newVaultAddress) public onlyOwner { 528 vault = _newVaultAddress; 529 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #13 534 function setWarriorsAddress( 535 IForgottenRunesWarriorsGuild _newWarriorsAddress 536 ) public onlyOwner { 537 warriors = _newWarriorsAddress; 538 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #14 543 function setWethAddress(address _newWethAddress) public onlyOwner { 544 weth = _newWethAddress; 545 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #15 550 function setStartPrice(uint256 _newPrice) public onlyOwner { 551 startPrice = _newPrice; 552 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #16 557 function setLowestPrice(uint256 _newPrice) public onlyOwner { 558 lowestPrice = _newPrice; 559 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #17 564 function setDaPriceCurveLength(uint256 _newTime) public onlyOwner { 565 daPriceCurveLength = _newTime; 566 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #18 571 function setDaDropInterval(uint256 _newTime) public onlyOwner { 572 daDropInterval = _newTime; 573 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #19 579 function setFinalPrice(uint256 _newPrice) public onlyOwner { 580 finalPrice = _newPrice; 581 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #20 586 function setMaxDaSupply(uint256 _newSupply) public onlyOwner { 587 maxDaSupply = _newSupply; 588 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #21 593 function setMaxForSale(uint256 _newSupply) public onlyOwner { 594 maxForSale = _newSupply; 595 }
File: /contracts/ForgottenRunesWarriorsMinter.sol #22 600 function setMaxForClaim(uint256 _newSupply) public onlyOwner { 601 maxForClaim = _newSupply; 602 }
Sometimes the plural form of the word is used as in claimsStartTime, but other times the singular is: claimlistMerkleRoot. The comments have similar issues as well. The code should use the same form for the same word.
File: /contracts/ForgottenRunesWarriorsMinter.sol (various lines) #1
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 20 /// @notice The start timestamp for mintlisters
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 41 /// @dev Having a backup merkle root lets us atomically update the merkletree without downtime on the frontend
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 44 /// @notice The claimslist Merkle root
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 476 * @param newMintlistStartTime uint256 the mintlst phase start time
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 518 * @notice set the merkle root for the claimslist phase
Code should follow the best-practice of check-effects-interaction
Reentrancy in ForgottenRunesWarriorsGuild.mint(address) (contracts/ForgottenRunesWarriorsGuild.sol#94-106): #1 External calls: - _safeMint(recipient,tokenId) (contracts/ForgottenRunesWarriorsGuild.sol#103) - IERC721Receiver(to).onERC721Received(_msgSender(),from,tokenId,_data) (node_modules/@openzeppelin/contracts/token/ERC721/ERC721.sol#376-386) State variables written after the call(s): - numMinted += 1 (contracts/ForgottenRunesWarriorsGuild.sol#104)
pause() and unpause() keep track of how long things were paused, and automatically add that amount of time to the contract timesThis will make the pause/unpause procedure less error-prone, and will ensure that the full duration of a phase is always maintained
File: contracts/ForgottenRunesWarriorsMinter.sol #1 427 function pause() public onlyOwner { 428 _pause(); 429 } 430 431 /** 432 * @notice unpause the contract 433 */ 434 function unpause() public onlyOwner { 435 _unpause(); 436 }
🌟 Selected for report: BowTiedWardens
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xProf, 0xc0ffEE, 0xf15ers, 0xkatana, 0xliumin, ACai, AlleyCat, CertoraInc, Cityscape, Cr4ckM3, DavidGialdi, Dinddle, FSchmoede, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, MiloTruck, Picodes, RoiEvenHaim, Tadashi, TerrierLover, TrungOre, VAD37, WatchPug, antonttc, catchup, defsec, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, ilan, joestakey, kebabsec, kenta, kenzo, marximimus, minhquanym, noobie, oyc_109, p4st13r4, pauliax, rajatbeladiya, reassor, rfa, robee, rotcivegaf, saian, samruna, shenwilly, shung, simon135, slywaters, sorrynotsorry, throttle, unforgiven, z3s
21.02 USDC - $21.02
For your specific request about gas savings during issueRefunds(), the best way to reduce costs is to get large gas refunds for deleting storage entries. Every storage slot deleted/set to zero results in a gas refund of ~12,000 gas (Rsclear(15000) - Gsreset(2900)), up to half of the total cost of the transaction. If instead of keeping around that data for users after they've been refunded, you delete them, you'll get this refund. Since you have five mappings per user, deleting all five would result in a refund of ~60,000 gas per user. If you are worried about losing old state and being unable to help users debug issues related to refunds, you can just emit events for every refund, which will cost at most 2,000 gas per user, and will be visible on all block explorers. If you store all five mappings as a single mapping of structs (as is mentioned below), deletes will be cheaper because you're only accessing one mapping storage slot rather than five, and the fetching of the data to delete will be cheaper because the booleans will be returned from a single storage slot. In addition, rather than iterating over the daMinters array forwards, you can go backwards and delete entries along the way, saving another ~12,000 gas per entry.
address mappings can be combined into a single mapping of an address to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Deletion is a lot more efficient
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 76 mapping(address => uint256) public daAmountPaid; 77 78 /// @notice Tracks the total amount refunded to a given address for the DA 79 mapping(address => uint256) public daAmountRefunded; 80 81 /// @notice Tracks the total count of NFTs minted by a given address in the DA 82 mapping(address => uint256) public daNumMinted; 83 84 /// @notice Tracks if a given address minted in the mintlist 85 mapping(address => bool) public mintlistMinted; 86 87 /// @notice Tracks the total count of NFTs claimed by a given address 88 mapping(address => bool) public claimlistMinted;
The instances below point to the second+ access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 102 uint256 tokenId = numMinted;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 287 uint256 elapsed = block.timestamp - daStartTime;
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 610 require(payable(vault).send(_amount));
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 618 require(payable(vault).send(address(this).balance));
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 402 IERC20(weth).transfer(to, amount);
File: /contracts/ForgottenRunesWarriorsMinter.sol #6 292 if (stepDeduction > startPrice) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #7 295 uint256 currentPrice = startPrice - stepDeduction;
File: /contracts/ForgottenRunesWarriorsMinter.sol #8 293 return lowestPrice;
File: /contracts/ForgottenRunesWarriorsMinter.sol #9 296 return currentPrice > lowestPrice ? currentPrice : lowestPrice;
File: /contracts/ForgottenRunesWarriorsMinter.sol #10 296 return currentPrice > lowestPrice ? currentPrice : lowestPrice;
File: /contracts/ForgottenRunesWarriorsMinter.sol #11 285 (daPriceCurveLength / daDropInterval);
daPriceCurveLength https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3//contracts/ForgottenRunesWarriorsMinter.sol#L285
File: /contracts/ForgottenRunesWarriorsMinter.sol #12 288 uint256 steps = elapsed / daDropInterval;
File: /contracts/ForgottenRunesWarriorsMinter.sol #13 137 require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
File: /contracts/ForgottenRunesWarriorsMinter.sol #14 137 require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
File: /contracts/ForgottenRunesWarriorsMinter.sol #15 156 if (numSold == maxDaSupply) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #16 208 require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
File: /contracts/ForgottenRunesWarriorsMinter.sol #17 208 require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 104 numMinted += 1;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 154 numSold += numWarriors;
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 193 numSold += 1;
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 219 numSold += numWarriors;
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 248 numClaimed += 1;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 399 function _safeTransferETHWithFallback(address to, uint256 amount) internal {
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 412 function _safeTransferETH(address to, uint256 value) 413 internal 414 returns (bool)
++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThis saves 30-40 gas PER LOOP
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 162 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 220 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 259 for (uint256 i = 0; i < count; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 355 for (uint256 i = startIdx; i < endIdx + 1; i++) {
require()/revert() strings longer than 32 bytes cost extra gasFile: /contracts/ForgottenRunesWarriorsGuild.sol #1 68 require( 69 _exists(tokenId), 70 'ERC721Metadata: URI query for nonexistent token' 71 );
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 114 require( 115 _isApprovedOrOwner(_msgSender(), tokenId), 116 'ERC721Burnable: caller is not owner nor approved' 117 );
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 140 require( 141 numWarriors > 0 && numWarriors <= 20, 142 'You can summon no more than 20 Warriors at a time' 143 );
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 146 require( 147 msg.value >= (currentPrice * numWarriors), 148 'Ether value sent is not sufficient' 149 );
File: /contracts/ForgottenRunesWarriorsMinter.sol #5 210 require( 211 numWarriors > 0 && numWarriors <= 20, 212 'You can summon no more than 20 Warriors at a time' 213 );
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
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 85 mapping(address => bool) public mintlistMinted;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 88 mapping(address => bool) public claimlistMinted;
Use a solidity version of at least 0.8.2 to get 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
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 1 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 1 pragma solidity ^0.8.0;
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 162 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 220 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 259 for (uint256 i = 0; i < count; i++) {
++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)File: /contracts/ForgottenRunesWarriorsMinter.sol #1 162 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 220 for (uint256 i = 0; i < numWarriors; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 259 for (uint256 i = 0; i < count; i++) {
File: /contracts/ForgottenRunesWarriorsMinter.sol #4 355 for (uint256 i = startIdx; i < endIdx + 1; i++) {
require() statements that use && saves gasSee this issue for an example
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 140 require( 141 numWarriors > 0 && numWarriors <= 20, 142 'You can summon no more than 20 Warriors at a time' 143 );
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 210 require( 211 numWarriors > 0 && numWarriors <= 20, 212 'You can summon no more than 20 Warriors at a time' 213 );
private rather than public for constants, saves gasIf needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 21 uint256 public constant MAX_WARRIORS = 16000;
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 32 string public constant R = 33 "Old men forget: yet all shall be forgot, But he'll remember with advantages What feats he did that day: then shall our names Familiar in his mouth as household words. This story shall the good man teach his son From this day to the ending of the world";
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 182 require(mintlistMinted[msg.sender] == false, 'Already minted');
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 238 require(claimlistMinted[msg.sender] == false, 'Already claimed');
require()/revert() checks should be refactored to a modifier or functionSaves deployment costs
File: /contracts/ForgottenRunesWarriorsMinter.sol #1 210 require( 211 numWarriors > 0 && numWarriors <= 20, 212 'You can summon no more than 20 Warriors at a time' 213 );
File: /contracts/ForgottenRunesWarriorsMinter.sol #2 207 require(numSold < maxForSale, 'Sold out');
File: /contracts/ForgottenRunesWarriorsMinter.sol #3 617 require(address(vault) != address(0), 'no vault');
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 152 function uploadImage(bytes calldata s) external onlyOwner {}
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 157 function uploadAttributes(bytes calldata s) external onlyOwner {}
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
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 52 function initialize(address newMinter) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 129 function setBaseURI(string memory newBaseURI) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #3 137 function setMinter(address newMinter) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #4 145 function setProvenanceHash(string memory newHash) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #5 152 function uploadImage(bytes calldata s) external onlyOwner {}
File: /contracts/ForgottenRunesWarriorsGuild.sol #6 157 function uploadAttributes(bytes calldata s) external onlyOwner {}
File: /contracts/ForgottenRunesWarriorsGuild.sol #7 163 function withdrawAll() public payable onlyOwner {
File: /contracts/ForgottenRunesWarriorsGuild.sol #8 173 function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #9 257 function teamSummon(address recipient, uint256 count) external onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #10 350 function issueRefunds(uint256 startIdx, uint256 endIdx) 351 public 352 onlyOwner 353 nonReentrant
File: /contracts/ForgottenRunesWarriorsMinter.sol #11 364 function refundAddress(address minter) public onlyOwner nonReentrant {
File: /contracts/ForgottenRunesWarriorsMinter.sol #12 427 function pause() public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #13 434 function unpause() public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #14 441 function setDaStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #15 448 function setMintlistStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #16 455 function setPublicStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #17 462 function setClaimsStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #18 469 function setSelfRefundsStartTime(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #19 480 function setPhaseTimes( 481 uint256 newDaStartTime, 482 uint256 newMintlistStartTime, 483 uint256 newPublicStartTime, 484 uint256 newClaimsStartTime 485 ) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #20 505 function setMintlist1MerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #21 513 function setMintlist2MerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #22 520 function setClaimlistMerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #23 527 function setVaultAddress(address _newVaultAddress) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #24 534 function setWarriorsAddress( 535 IForgottenRunesWarriorsGuild _newWarriorsAddress 536 ) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #25 543 function setWethAddress(address _newWethAddress) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #26 550 function setStartPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #27 557 function setLowestPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #28 564 function setDaPriceCurveLength(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #29 571 function setDaDropInterval(uint256 _newTime) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #30 579 function setFinalPrice(uint256 _newPrice) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #31 586 function setMaxDaSupply(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #32 593 function setMaxForSale(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #33 600 function setMaxForClaim(uint256 _newSupply) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #34 608 function withdraw(uint256 _amount) public onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #35 616 function withdrawAll() public payable onlyOwner {
File: /contracts/ForgottenRunesWarriorsMinter.sol #36 627 function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
_msgSender() if not supporting EIP-2771Use msg.sender if the code does not implement EIP-2771 trusted forwarder support
File: /contracts/ForgottenRunesWarriorsGuild.sol #1 101 require(_msgSender() == minter, 'Not a minter');
File: /contracts/ForgottenRunesWarriorsGuild.sol #2 115 _isApprovedOrOwner(_msgSender(), tokenId),
Before starting the for-loop, you should do unchecked { ++endIdx } so that you aren't doing the addition during every loop
File: contracts/ForgottenRunesWarriorsMinter.sol #1 355 for (uint256 i = startIdx; i < endIdx + 1; i++) {
Calculate dropPerStep in a state variable whenever one of the constituent values changes, and use the state variable in this function
File: contracts/ForgottenRunesWarriorsMinter.sol #2 284 uint256 dropPerStep = (startPrice - lowestPrice) / 285 (daPriceCurveLength / daDropInterval);