Platform: Code4rena
Start Date: 22/09/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 133
Period: 3 days
Judge: 0xean
Total Solo HM: 2
Id: 165
League: ETH
Rank: 19/133
Findings: 4
Award: $184.14
🌟 Selected for report: 0
🚀 Solo Findings: 0
The size of minters_array is never reduced because when a minter is "removed", the corresponding element is simply set to zero address. At some point, the loop can run out of gas, and the removeMinter() function becomes unavailable.
minters_array is not limited in size.
Manual review.
minters_array at all, since the minters' addresses are already stored in the mapping minters.minters_array.#0 - FortisFortuna
2022-09-25T22:41:00Z
Technically correct, but in practice, the number of minters will always remain low. If it becomes an issue, we can designate one minter as a "pre-minter" that has a batch of tokens minted to it beforehand, then auxiliary contracts can connect to that instead of ERC20PermitPermissionedMint.sol instead.
#1 - joestakey
2022-09-26T16:22:24Z
Duplicate of #12
39.1574 USDC - $39.16
https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L125-L154
The number of iterations in the loop depends on the balance of the contract. The larger the balance of the contract, the more iterations will be in the loop, and then:
depositEther() will revert.depositEther() will revert. (getNextValidator(); // Will revert if there are not enough free validators)So, the function depositEther() will become unavailable until the contract balance is decreased and/or new validators are added.
https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L125-L154
Manual review.
Consider limiting the number of iterations per function call.
For example, you can declare the constant MAX_NUM_DEPOSITS and add the if-statement before the loop:
if (numDeposits > MAX_NUM_DEPOSITS) { numDeposits = MAX_NUM_DEPOSITS; }
Then, in case of a large contract balance, the function will not get stuck.
#0 - FortisFortuna
2022-09-26T16:30:05Z
Adding a maxLoops parameter or similar can help mitigate this for sure.
#1 - FortisFortuna
2022-09-26T17:21:58Z
🌟 Selected for report: rotcivegaf
Also found by: 0x040, 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, B2, Bahurum, Bnke0x0, Ch_301, CodingNameKiki, Deivitto, Diana, Funen, IllIllI, JC, JLevick, KIntern_NA, Lambda, OptimismSec, PaludoX0, RockingMiles, Rolezn, Sm4rty, Soosh, Tagir2003, Tointer, TomJ, Triangle, Trust, V_B, Waze, Yiko, __141345__, a12jmx, ajtra, asutorufos, ayeslick, aysha, bbuddha, bharg4v, bobirichman, brgltd, bytera, c3phas, cryptostellar5, cryptphi, csanuragjain, datapunk, delfin454000, durianSausage, exd0tpy, gogo, got_targ, jag, joestakey, karanctf, ladboy233, leosathya, lukris02, mics, millersplanet, natzuu, neko_nyaa, obront, oyc_109, parashar, peritoflores, rbserver, ret2basic, rokinot, ronnyx2017, rvierdiiev, sach1r0, seyni, sikorico, slowmoses, tnevler, yasir, yongskiws
29.0054 USDC - $29.01
During the audit, 3 low and 4 non-critical issues were found.
| № | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Large number of elements may cause out-of-gas error | Low | 3 |
| L-2 | Check zero denominator | Low | 1 |
| L-3 | Missing check | Low | 1 |
| NC-1 | Order of Layout | Non-Critical | 3 |
| NC-2 | Floating pragma | Non-Critical | 6 |
| NC-3 | Missing NatSpec | Non-Critical | 7 |
| NC-4 | Public functions can be external | Non-Critical | 10 |
Loops that do not have a fixed number of iterations, for example, loops that depend on storage values, have to be used carefully: Due to the block gas limit, transactions can only consume a certain amount of gas. Either explicitly or just due to normal operation, the number of iterations in a loop can grow beyond the block gas limit, which can cause the complete contract to be stalled at a certain point.
Restrict the maximum number of elements.
If the input parameter is equal to zero, this will cause the call failure on division.
Add the check to prevent function call failure.
No check that times <= validators.length. Without check, the function will try to pop more elements than there are in the array.
Add require statement or custom error - times <= validators.length.
According to Order of Layout, inside each contract, library or interface, use the following order:
Events should not be at the end of the contract:
Place events before modifiers.
Contracts should be deployed with the same compiler version. It helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
According to SWC-103, pragma version should be locked.
NatSpec is missing for 7 functions in 2 contracts.
Add NatSpec for all functions.
If functions are not called by the contract where they are defined, they can be declared external.
Make public functions external, where possible.
🌟 Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x4non, 0x5rings, 0xA5DF, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Amithuddar, Aymen0909, B2, Ben, Bnke0x0, Chom, CodingNameKiki, Deivitto, Diana, Fitraldys, Funen, IllIllI, JAGADESH, JC, Metatron, Ocean_Sky, PaludoX0, Pheonix, RaymondFam, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Satyam_Sharma, Sm4rty, SnowMan, SooYa, Tagir2003, TomJ, Tomio, Triangle, V_B, Waze, __141345__, ajtra, albincsergo, asutorufos, aysha, beardofginger, bobirichman, brgltd, bulej93, bytera, c3phas, ch0bu, cryptostellar5, cryptphi, d3e4, delfin454000, dharma09, drdr, durianSausage, emrekocak, erictee, fatherOfBlocks, gogo, got_targ, imare, jag, karanctf, ladboy233, leosathya, lukris02, medikko, mics, millersplanet, natzuu, neko_nyaa, oyc_109, peanuts, prasantgupta52, rbserver, ret2basic, rokinot, ronnyx2017, rotcivegaf, sach1r0, samruna, seyni, slowmoses, tnevler, wagmi, zishansami
12.8158 USDC - $12.82
During the audit, 8 gas issues were found.
Prefix increment costs less gas than postfix.
Consider using prefix increment where it is relevant.
Reading the length of an array at each iteration of the loop consumes extra gas.
Store the length of an array in a variable before the loop, and use it.
It costs gas to initialize integer variables with 0 or bool variables with false but it is not necessary.
Remove initialization for default values.
For example:
for (uint256 i; i < array.length; ++i) {
> 0 is more expensive than =! 0Use =! 0 instead of > 0, where possible.
x += y is more expensive than x = x + yUse x = x + y instead of x += y.
Use x = x - y instead of x -= y.
In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. When an overflow or underflow isn’t possible, some gas can be saved by using unchecked blocks.
Change:
for (uint256 i; i < n; ++i) { // ... }
to:
for (uint256 i; i < n;) { // ... unchecked { ++i; } }
Public is more expensive than private for constantsUse private constants instead of public, where possible.
Custom errors from Solidity 0.8.4 are cheaper than revert strings.
Use pragma 0.8.4+, and for example, change:
require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
to:
error NotOwnerOrTimelock(); ... if (!(msg.sender == timelock_address || msg.sender == owner)) revert NotOwnerOrTimelock();