Platform: Code4rena
Start Date: 21/06/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 96
Period: 3 days
Judge: HardlyDifficult
Total Solo HM: 5
Id: 140
League: ETH
Rank: 14/96
Findings: 2
Award: $1,080.98
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: PwnedNoMore
Also found by: ych18
1052.705 USDC - $1,052.70
uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));
_secondaryReserveRatio can be overflowed by setting a relatively small _initialTokenSupply and _initialTokenPrice. The result will be truncated by uint32, causing an overflow.
This overflow can bypass all the checks in function initialize. Any following functionality will be impacted since the _secondaryReserveRatio is incorrect.
_initialTokenSupply and _initialTokenPrice, which meets SCALE * 1e18 == _initialTokenSupply * _initialTokenPricemsg.value is set as 2 ** 32 + X, where MIN_SECONDARY_RESERVE_RATIO <= X <= primaryReserveRatio. Note that msg.value is in Wei, so the deposited fund is not huge.Add overflow checks.
#0 - HardlyDifficult
2022-07-04T01:06:34Z
OpenZeppelin has safe cast helpers that could be leveraged here.
It is concerning that due to the truncation here, the configuration would not work how the user expects given the input parameters. And at this point the NFT has been escrowed into the vault. Because of this it seems Medium risk is a fair assessment.
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xkatana, BowTiedWardens, Chom, ElKu, Funen, GalloDaSballo, JC, JMukesh, JohnSmith, Lambda, Limbooo, MadWookie, MiloTruck, Nethermind, Noah3o6, Nyamcil, Picodes, PwnedNoMore, Randyyy, RoiEvenHaim, SmartSek, StErMi, Tadashi, TerrierLover, TomJ, Tomio, Treasure-Seeker, UnusualTurtle, Varun_Verma, Wayne, Waze, _Adam, apostle0x01, asutorufos, berndartmueller, c3phas, catchup, cccz, cloudjunky, codexploder, cryptphi, defsec, delfin454000, dipp, ellahi, exd0tpy, fatherOfBlocks, hansfriese, hyh, joestakey, kebabsec, kenta, masterchief, minhquanym, naps62, oyc_109, pashov, peritoflores, reassor, rfa, robee, sach1r0, saian, sashik_eth, shenwilly, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, ych18, zuhaibmohd, zzzitron
28.2785 USDC - $28.28
Function permit allows users to sign an approval for another user. It leverages _approve function to set a new allowance, which is infamously vulnerable from double spending attacks. It is even much easier to launch the attack since the signed message can be first sent to the malicious user.
It is not recommended to directly set the new allowance, which is already advised by many security auditor. A malicious user can spend much more than the benign one excepts.
permit to reset her allowance to 500 tokensSince the safe decreaseAllowance and increaseAllowance are already supported by ERC20Upgradeable, it is recommended to use these two.
#0 - GalloDaSballo
2022-06-26T22:00:18Z
Finding claims that _approve race condition is High Severity, issue has been historically Low / Non-Critical as it's Ethereum Consensus Based
#1 - HardlyDifficult
2022-06-30T13:42:30Z
This implementation of permit has become industry standard, e.g. see https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol#L74. Maintaining consistency may be important for integrations and ease of use/understanding. This report appears to be a general concern with the permit standard and not specific to Nibbl - so this seems out of scope, particularly for a High risk submission.
It is fair to suggest adding a permitIncrease type function for the reason outlined here. I'm lowering the risk and making this submission a QA report for this warden.
#2 - HardlyDifficult
2022-07-01T17:18:00Z