Platform: Code4rena
Start Date: 26/07/2022
Pot Size: $75,000 USDC
Total HM: 29
Participants: 179
Period: 6 days
Judge: LSDan
Total Solo HM: 6
Id: 148
League: ETH
Rank: 130/179
Findings: 1
Award: $35.17
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0x52, 0xA5DF, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xc0ffEE, 0xf15ers, 0xmatt, 0xsanson, 0xsolstars, 8olidity, AuditsAreUS, Bahurum, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chom, CryptoMartian, Deivitto, DevABDee, Dravee, ElKu, Franfran, Funen, GalloDaSballo, GimelSec, GiveMeTestEther, Green, JC, Jmaxmanblue, JohnSmith, Jujic, Junnon, Kenshin, Krow10, Kumpa, Lambda, MEP, Maxime, MiloTruck, Mohandes, NoamYakov, Picodes, RedOneN, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, Soosh, StErMi, StyxRave, Tadashi, TomJ, Treasure-Seeker, TrungOre, Waze, _Adam, __141345__, ajtra, ak1, apostle0x01, arcoun, asutorufos, async, benbaessler, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, carlitox477, chatch, codetilda, codexploder, cryptonue, cryptphi, csanuragjain, cthulhu_cult, delfin454000, dipp, dirk_y, djxploit, ellahi, exd0tpy, fatherOfBlocks, giovannidisiena, hansfriese, horsefacts, hyh, idkwhatimdoing, indijanc, jayfromthe13th, jayphbee, joestakey, kenzo, kyteg, lucacez, luckypanda, mics, minhquanym, obront, oyc_109, pedr02b2, rajatbeladiya, rbserver, reassor, robee, rokinot, rotcivegaf, sach1r0, saian, saneryee, sashik_eth, scaraven, shenwilly, simon135, sseefried, supernova, teddav, ych18, zuhaibmohd, zzzitron
35.1687 USDC - $35.17
Details: As stated in Solidity docs, ecrecover in L176 of GolomTrader.sol may return 0 on error (e.g. passing o.v different than 26 or 27). This means that the require in L177 can be bypassed by passing an Order with o.signer equal to 0.
Impact: Low? (I am marking it as a low because I couldn’t find how to exploit this to cause a high-impact attack to the contract or its users. Nevertheless, there could be a possibility that this could be combined with other potential attacks to cause high damage).
Mitigation: Add a require condition to revert the transaction in case ecrecover returns 0.
Details: GolomTrader.sol defines chainId at contract deployment without reconstructing it for every signature. However, as stated in the security considerations section of EIP2612, "If the DOMAIN_SEPARATOR contains the chainId and is defined at contract deployment instead of reconstructed for every signature, there is a risk of possible replay attacks between chains in the event of a future chain split.” ****
Impact: Low (depends on the hard forking of a well-established blockchain)
Mitigation: Consider caching the chainId as is done in OpenZeppelin contracts.
Details: The comment in L16 of TokenUriHelper.sol does not match the operation in L17:
// multiply by 4/3 rounded up uint256 encodedLen = 4 * ((len + 2) / 3);
Indeed, consider the following counter-example: if len is equal to 3, then 4/3 of 5 is, after rounding up, 7. However, the resulting value of encodedLen is 4.
Impact: Code QA
Details: Since integer division might truncate, performing multiplication before division may avoid precision loss. Consider changing the order of multiplication and division in the following lines:****
L381 of GolomTrader.sol to
uint256 protocolfee = ((o.totalAmt * amount * 50) / 10000);
Impact: Code QA
Mitigation: Perform the changes detailed above.