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: 34/93
Findings: 3
Award: $108.74
🌟 Selected for report: 0
🚀 Solo Findings: 0
48.5872 USDC - $48.59
Judge has assessed an item in Issue #119 as Medium risk. The relevant finding follows:
.call() instead of .send()It is recommended to use call() instead of send() because the former fowards all remaining gas with the call, while the latter has a gas limit of 2300. This would cause withdrawals to fail if the recipient (vault) is a smart contract that:
receive() / payable fallback() functions(bool success, ) = payable(vault).call{ value: _amount }(""); require(success, "ETH transfer failed");
#0 - gzeoneth
2022-06-18T19:19:22Z
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
35.025 USDC - $35.02
Codebase quality in general is high. The accompanying video walkthrough and explanation greatly helps us wardens to be quickly acquainted with the various functionalities. Contracts are also well-tested and covered.
The greatest risk I see is centralisation. The dutch auction (DA) parameters, phase timestamps, final DA price and WETH and warrior NFT contracts are mutable, and lack any restrictions on when they can be called. All funds can also be withdrawn all funds at any time. The team is also able to mint as many NFTs as they would like (up to the hardcapped supply of 16k).
From an operational perspective, I understand the need for things to be kept fluid so that the team will be able to respond and update to situations. However, the downside of this is that it is impossible to introduce safeguards into the system. For instance, it would be great to have a sanity limit on setFinalPrice where it can’t exceed startPrice, but this sanity limit is rendered ineffective because startPrice is mutable. Consider relinquishing control over some less important variables to reduce the trust people need to have on the team.
.call() instead of .send()It is recommended to use call() instead of send() because the former fowards all remaining gas with the call, while the latter has a gas limit of 2300. This would cause withdrawals to fail if the recipient (vault) is a smart contract that:
receive() / payable fallback() functions(bool success, ) = payable(vault).call{ value: _amount }(""); require(success, "ETH transfer failed");
setWarriorsAddress and setWethAddress settersHaving setters for the warrior NFT and WETH addresses doesn’t exactly inspire confidence over the code quality. It would give readers the impression that they can be changed to another address at any time.
Remove these setters and make the variables immutable since they will only be set in the constrcutor.
The team is able to mint any amount of warriors to any desired recipient at once, up to the hardcoded supply of 16k. This could potentially cause griefing for users participating in the different phases.
Consider limiting the number of NFTs that the team can mint (16k - maxForSale - maxForClaim). Admittedly, having this limit is rendered less effective by the fact that maxForSale and maxForClaim are mutable.
🌟 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
25.132 USDC - $25.13
Some gas optimizations can be achieved by simply updating the settings in hardhat.config.ts.
2**32-1). More information can be found in the solidity docs.0.8.13 ”can do much higher-level optimization across functions”.hardhat.config.ts. Contract sizes and thus deployment costs will increase, but functions will cost less gas to execute.solidity: { compilers: [ { version: '0.8.13', settings: { viaIR: true, // use IR-based Yul optimizer optimizer: { enabled: true, runs: 100000 }, // increase solc runs }, }, ...
msg.sender will be the owner’s address, which is non-zero. The non-zero address check in forwardERC20s is therefore redundant and can be removed.
require(address(msg.sender) != address(0));
daMinters array only if amountPaid is zero to avoid duplicatesTo avoid storing duplicate addresses in the daMinters array (thus avoiding a potential SSTORE and saves gas when issuing refunds), consider pushing the minter address only for the first mint. This can be checked in multiple ways, such as if either daAmountPaid or daNumMinted is non-zero.
if (daAmountPaid[msg.sender] == 0) daMinters.push(msg.sender);
Since claimlistMinted[msg.sender] is a boolean mapping, there isn’t a need to check against the boolean literal false.
require(!claimlistMinted[msg.sender], 'Already claimed');
WETH to IWETH type to avoid repeated castingBy declaring WETH to be of IWETH, repeated castings to IWETH and IERC20 can be avoided. For instance, the gas cost for the should refund a griefer test reduces by 124 gas from 144654 to 144530.
IWETH public weth; function setWethAddress(address _newWethAddress) public onlyOwner { weth = IWETH(_newWethAddress); } function _safeTransferETHWithFallback(address to, uint256 amount) internal { if (!_safeTransferETH(to, amount)) { weth.deposit{value: amount}(); weth.transfer(to, amount); } }
1 instead of 0As outlined in the 4th tip of this NFT gas optimization article, “if one of your users is going to make the first mint, make it cheaper for them by initializing the tokenId to 1.”
function mint(address recipient) public override nonReentrant returns (uint256) { // note that < has been changed to <= // to account for shift in starting token ID require(numMinted <= MAX_WARRIORS, 'All warriors have been summoned'); require(_msgSender() == minter, 'Not a minter'); uint256 tokenId = ++numMinted; _safeMint(recipient, tokenId); return tokenId; }
new bytes(0) with ""It is more gas efficient to use "" as an argument to provide empty calldata instead of new bytes(0). The gas reporter reported a ~5k gas saving for the “should have reasonable gas to refund” test.
(bool success, ) = to.call{value: value, gas: 30_000}("");