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: 65/93
Findings: 2
Award: $45.77
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
30.2759 USDC - $30.28
Team actions don't have any limitations. This can look a little bit suspicious, the users will have to trust the protocol owners and in my opinion it'll be better to add limitations to this actions.
🌟 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
15.491 USDC - $15.49
variables in solidity are initialized automatically to their default value, and it actually costs more gas to initialize them manually, so the numMinted
variable in the ForgottenRunesWarriorsGuild
contract shouldn't be initialized
uint256 public numMinted; // uint256 public numMinted = 0; // costs more gas
Use unchecked in the bidSummon
function - we know for sure that the first expression won't overflow because currentPrice * numWarriors <= startingPrice * 20 < 2**256 - 1
, and the second calculations won't overflow because daNumMinted[msg.sender] + numWarriors <= numSold + numWarriors <= max supply < 2**256 - 1
require( msg.value >= (currentPrice * numWarriors), 'Ether value sent is not sufficient' ); // ... daNumMinted[msg.sender] += numWarriors; numSold += numWarriors;
Use unchecked in the currentDaPrice
function - we know that stepDeduction <= startPrice
so startPrice - stepDeduction
won't underflow
if (stepDeduction > startPrice) { return lowestPrice; } uint256 currentPrice = startPrice - stepDeduction;
Loops can be optimized in several ways. Let's take for example the loop in the bidSummon
, teamSummon
and in the publicSummon
functions
for (uint256 i = 0; i < numWarriors; i++) { _mint(msg.sender); }
We can do multiple things here:
uint i;
instead of uint i = 0;
.for (uint256 i; i < numWarriors; ) { _mint(msg.sender); unchecked { ++i; } }
There is another loop in the issueRefunds
function you can apply similar changes to in order to optimize it.
Gas optimizations of the issueRefunds
function (and some helper functions it uses)
I optimized this function with multiple optimizations, so I'll describe them in steps, so it'll be easier for you to review each optimization alone and use the ones you want.
This is the code of the original function with the helper functions it uses:
function issueRefunds(uint256 startIdx, uint256 endIdx) public onlyOwner nonReentrant { for (uint256 i = startIdx; i < endIdx + 1; i++) { _refundAddress(daMinters[i]); } } function _refundAddress(address minter) private { uint256 owed = refundOwed(minter); if (owed > 0) { daAmountRefunded[minter] += owed; _safeTransferETHWithFallback(minter, owed); } } function refundOwed(address minter) public view returns (uint256) { uint256 totalCostOfMints = finalPrice * daNumMinted[minter]; uint256 refundsPaidAlready = daAmountRefunded[minter]; return daAmountPaid[minter] - totalCostOfMints - refundsPaidAlready; } /** * @notice Transfer ETH. If the ETH transfer fails, wrap the ETH and try send it as WETH. * @param to account who to send the ETH or WETH to * @param amount uint256 how much ETH or WETH to send */ function _safeTransferETHWithFallback(address to, uint256 amount) internal { if (!_safeTransferETH(to, amount)) { IWETH(weth).deposit{value: amount}(); IERC20(weth).transfer(to, amount); } } /** * @notice Transfer ETH and return the success status. * @dev This function only forwards 30,000 gas to the callee. * @param to account who to send the ETH to * @param value uint256 how much ETH to send */ function _safeTransferETH(address to, uint256 value) internal returns (bool) { (bool success, ) = to.call{value: value, gas: 30_000}(new bytes(0)); return success; }
The starting gas cost from the report is 69745 - 143756 (Min = 69745, Max = 143756).
The first optimization is the loop optimization that I described before. These optimizations reduce the cost to 69531 - 143542.
The second optimization is that instead of using the daAmountRefunded
mapping, you can simply subtract from the daAmountPaid
value of the user.
This optimization will make the contract use less storage and make the gas cost of the function a lot cheaper, because the reduction in the storage usage and because the refundOwed
function will return daAmountPaid[minter] - totalCostOfMints
instead of daAmountPaid[minter] - totalCostOfMints - refundsPaidAlready
which is a simpler calculation.
This optimization reduces the gas cost to 50181 - 124192, which is a big change compared to the previous cost.
Inline the safeTransferEthWithFallback and safeTransferEth function, which means write their code inside the function instead of calling the functions.
This reduces the cost to 50098 - 124097.
Use unchecked in the _refundAddress function - the calculations will be something like
uint newTotalPaid = finalPrice * daNumMinted[minter]; uint owed = totalPaid - newTotalPaid;
and you can use unchecked to make them cheaper (we know it won't overflow or underflow because we know that finalPrice * daNumMinted[minter] <= newTotalPaid
).
This reduces the cost to 50009 - 124008.
Replace the bytes(0)
with ""
in the low level call.
Inline the _refundAddress and refundOwed functions, and also optimize the code. Some optimization can be done there because the inlined code uses the same (storage) values that can be saved in order to use less storage.
The new code will look something like this:
function issueRefunds(uint256 startIdx, uint256 endIdx) public onlyOwner nonReentrant { unchecked { for (uint256 i = startIdx; i <= endIdx; ) { // _refundAddress(daMinters[i]); address minter = daMinters[i]; uint totalPaid = daAmountPaid[minter]; // uint256 owed = refundOwed(minter); uint newTotalPaid = finalPrice * daNumMinted[minter]; uint owed = totalPaid - newTotalPaid; if (owed > 0) { // daAmountRefunded[minter] += owed; daAmountPaid[minter] = newTotalPaid; // _safeTransferETHWithFallback(, owed); (bool success, ) = minter.call{value: owed, gas: 30_000}(""); if (!success) { IWETH(weth).deposit{value: owed}(); IERC20(weth).transfer(minter, owed); } } ++i; } } }
The final cost of this implementation is 49485 - 123444, which is ~29% improvement on the minimum cost, and ~14% on the maximum cost.