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: 45/93
Findings: 2
Award: $78.87
🌟 Selected for report: 0
🚀 Solo Findings: 0
48.5872 USDC - $48.59
Same as https://github.com/code-423n4/2022-01-openleverage-findings/issues/75
When the owner calls the withdraw* function to withdraw native token, the whole user withdraw is being handled with a payable.send() call.
This is unsafe as send has hard coded gas budget and can fail when the user is a smart contract.
Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token send exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L608-L611 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L616-L619 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L163-L165 https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
None
Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60
#0 - KenzoAgada
2022-06-06T12:49:30Z
Duplicate of #254
#1 - gzeoneth
2022-06-18T17:23:51Z
This is true but also these are onlyOwner function where the owner have full control of the destination address to mitigate any issue in production. Downgrading to Low / QA.
🌟 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.2807 USDC - $30.28
The initialize function in ForgottenRunesWarriorsGuild contract is not protected with state variables, so the initialize function can be called multiple times.
None
Set the state variable in the initialize function so that the initialize function can only be called once
Same as https://github.com/code-423n4/2021-09-swivel-findings/issues/101 and https://github.com/code-423n4/2021-11-overlay-findings/issues/120
onlyOwner functions that change critical contract parameters/addresses/state should emit events and consider adding timelocks so that users and other privileged roles can detect upcoming changes (by offchain monitoring of events) and have the time to react to them
https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L129-L147 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L441-L602
None
Consider using a timelock for critical params of the system and emitting events to inform the outside world.