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: 15/93
Findings: 3
Award: $622.23
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: pedroais
Also found by: AuditsAreUS, BowTiedWardens, GimelSec, IllIllI, WatchPug, defsec, leastwood
542.7657 USDC - $542.77
https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L469-L471 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L372
The self refund feature is implemented to allow users who bid in the dutch auction the difference between their total bid price and the final price. This ensures that all auction participants paid the same amount for the NFT. However, if setSelfRefundsStartTime() is called at any point during the auction, it can potentially allow the bidder to self refund early at a loss or put the contract in a state where noone can refund what they are owed.
Consider preventing calls to setSelfRefundsStartTime() when the auction has started. It might be good to ensure that no setter functions can be called once an auction has started. This ensures that the governance cannot tamper with an ongoing auction.
#0 - cryppadotta
2022-05-06T15:44:59Z
In what way does this allow the bidder to self-refund early at a loss or cause an invalid state?
I believe the self refund logic is sound enough that it could be enabled even while the auction is ongoing, but I've disabled that just because I'm scared, tbh.
#1 - gzeoneth
2022-06-18T18:50:45Z
Duplicate of #38
48.5872 USDC - $48.59
https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L610 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L618 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L164
The .send() function intends to transfer an ETH amount with a fixed amount of 2300 gas. This function is not equipped to handle changes in the underlying .send() and .transfer() functions which may supply different amounts of gas in the future. Additionally, if the recipient implements a fallback function containing some sort of logic, this may inevitably revert, meaning the vault and owner of the contract will never be able to call certain sensitive functions.
Consider using .call() instead with the checks-effects-interactions pattern implemented correctly. Careful consideration needs to be made to prevent reentrancy.
#0 - gzeoneth
2022-06-18T19:16:12Z
Determined the stake is high here and therefore Med Risk instead of Low.
🌟 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.8695 USDC - $30.87
The pause mechanism is implemented to allow the governance to the pause the contract under certain conditions. However, if the governance (onlyOwner) role is held behind a timelock contract under a multisig, then there could be considerable delay before a pause action has taken place. Under bad circumstances, funds could have already been stolen at this point.
It makes sense to have a list of guardians who are trusted by the team, but are only able to pause the contract. Alternatively, a more succinct solution would be to have a way to track certain invariants within the contract. This could be that the total funds in the contract must exceed the number of NFTs minted * the current price before the auction has ended. A combination of the two may be most helpful, considering we can enforce the contract is in a good state but allow a subset of guardians to pause the contract if they notice other dodgy behaviour.
Consider utilising the aforementioned guardians list or allow anyone to pause the contract if certain invariants are met.
#0 - cryppadotta
2022-05-06T15:46:21Z
We're going to use a multisig without timelock so this isn't a real issue
#1 - gzeoneth
2022-06-18T18:49:00Z
Downgrading to Low / QA
#2 - gzeoneth
2022-06-18T19:35:07Z
Consider as warden's QA report.