Platform: Code4rena
Start Date: 06/09/2022
Pot Size: $90,000 USDC
Total HM: 33
Participants: 168
Period: 9 days
Judge: GalloDaSballo
Total Solo HM: 10
Id: 157
League: ETH
Rank: 106/168
Findings: 2
Award: $66.38
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: MEP
Also found by: 0xSky, CertoraInc, MiloTruck, PwnPatrol, R2, Tointer, V_B, __141345__, antonttc, azephiar, cccz, d3e4, datapunk, davidbrai, easy_peasy, hansfriese, minhtrng, neumo, pcarranzav, peritoflores, scaraven, teawaterwire, tonisives, unforgiven, wagmi, zkhorse, zzzitron
5.6134 USDC - $5.61
https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L71-L126 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L118
Function _addFounders in Token.sol does not always allocate the correct percentage of NFTs for each founder.
In the test below (which I included inside Token.t.sol test file), there are two founders with 30 and 50 percent of allocation respectively, but in the end the allocation is only 35% for the 50 percent founder.
function test_FoundersPercentIssue() public { // Deploy address[] memory wallets = new address[](2); uint256[] memory percents = new uint256[](2); uint256[] memory vestingEnds = new uint256[](2); wallets[0] = founder; wallets[1] = founder2; percents[0] = 30; percents[1] = 50; vestingEnds[0] = 4 weeks; vestingEnds[1] = 4 weeks; setFounderParams(wallets, percents, vestingEnds); setMockTokenParams(); setMockAuctionParams(); setMockGovParams(); deploy(foundersArr, tokenParams, auctionParams, govParams); // end Deploy assertEq(token.totalFounders(), 2); assertEq(token.totalFounderOwnership(), 80); address wallet; uint8 numFounder; uint8 numFounder2; for(uint256 i;i<100;i++){ (wallet,,) = token.tokenRecipient(i); if(wallet == founder){ numFounder++; } else if(wallet == founder2){ numFounder2++; } } assertEq(numFounder, 30); assertEq(numFounder2, 35); }
Forge
The buggy line is this, because when baseTokenId is incremented by schedule and goes beyond 100, the token is not allocated to the founder because only the first 100 are counted towards allocation and minting in function _isForFounder. The las part of the line, % 100 doesn't affect the value of baseTokenId.
Instead of:
(baseTokenId += schedule) % 100;
it should be:
baseTokenId = (baseTokenId + schedule) % 100;
#0 - GalloDaSballo
2022-09-20T12:57:28Z
Dup of #499
🌟 Selected for report: Lambda
Also found by: 0x1337, 0x1f8b, 0x4non, 0x85102, 0xA5DF, 0xNazgul, 0xSmartContract, 0xbepresent, 0xc0ffEE, 8olidity, Aymen0909, B2, Bnke0x0, CRYP70, Captainkay, CertoraInc, Ch_301, Chom, ChristianKuri, CodingNameKiki, Deivitto, Diana, DimitarDimitrov, ElKu, EthLedger, Franfran, Funen, GimelSec, JansenC, Jeiwan, Jujic, Lead_Belly, MEP, MasterCookie, MiloTruck, Noah3o6, PPrieditis, PaludoX0, Picodes, PwnPatrol, R2, Randyyy, RaymondFam, Respx, ReyAdmirado, Rolezn, Samatak, Tointer, Tomo, V_B, Waze, _Adam, __141345__, a12jmx, ak1, asutorufos, azephiar, ballx, bharg4v, bin2chen, bobirichman, brgltd, bulej93, c3phas, cccz, ch0bu, cloudjunky, cryptonue, cryptostellar5, cryptphi, csanuragjain, d3e4, datapunk, davidbrai, delfin454000, dharma09, dic0de, dipp, djxploit, eierina, erictee, fatherOfBlocks, gogo, hansfriese, hyh, imare, indijanc, izhuer, jonatascm, ladboy233, leosathya, lucacez, lukris02, m9800, martin, minhtrng, ne0n, neumo, oyc_109, p_crypt0, pashov, pauliax, pcarranzav, pedr02b2, peritoflores, pfapostol, rbserver, ret2basic, robee, rvierdiiev, sach1r0, sahar, scaraven, sikorico, simon135, slowmoses, sorrynotsorry, tnevler, tonisives, volky, yixxas, zkhorse, zzzitron
60.7742 USDC - $60.77
https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L147-L150 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L146 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323-L327
There is an unchecked block that in case timeBuffer is sufficiently large, would make the sum overflow and set the endTime of the auction in the past, making the auction end automatically. The developers are aware of this, that's why they have this comment. But I think it's not a matter of realistically overflowing, it could be set to a value large enough by mistake. It's not worth it to not validate the value of the time buffer, because the consequences could be devastating.
The best option would be to validate in function setTimeBuffer that the timeBuffer cannot be set to a large value.
Manual analysis
Change function setTimeBuffer with this:
function setTimeBuffer(uint256 _timeBuffer) external onlyOwner { require(_timeBuffer < 31536000, "TimeBuffer: too big"); settings.timeBuffer = SafeCast.toUint40(_timeBuffer); emit TimeBufferUpdated(_timeBuffer); }
I supposed that timeBuffer should be less than one year (probably much less), so I compared here with the number of seconds in a year.
#0 - GalloDaSballo
2022-09-20T23:16:27Z
We have to acknowledge that a zero timeBuffer means each new auction will sell to the first bidder, while unorthodox this is a type of expression that certain projects may want to have for a certain period of time
Because of that I don't believe that a vulnerability was shown here
#1 - neumoxx
2022-09-22T10:10:20Z
@GalloDaSballo A zero timeBuffer has the impact of not extending the auction beyond the end time, It does not imply that it will sell to the first bidder. The issue here is that an overflow by setting the timeBuffer to a high enough value, could set the endTime of the auction in the past.
#2 - GalloDaSballo
2022-09-22T14:01:48Z
@GalloDaSballo A zero timeBuffer has the impact of not extending the auction beyond the end time, It does not imply that it will sell to the first bidder. The issue here is that an overflow by setting the timeBuffer to a high enough value, could set the endTime of the auction in the past.
Thank you for the comment, I'll double check, at this time I got these type of reports bulked as "unchecked validation of auction params", but I'll have to separate them with nuance as I spend more time with all the findings
#3 - iainnash
2022-09-26T19:08:58Z
I think what @neumoxx wrote is more concerning than the submission. I'd say the submission as-is is closer to a QA.
#4 - GalloDaSballo
2022-09-26T19:41:36Z
I think the submission has validity and unfortunately is not as developed as it could have.
By thinking about it, if settings.duration < settings.timeBuffer with timeBuffer big enough to cause and overflow, then any new auction would be created, and it would be basically ended after starting.
Assuming all those values are non-zero then this would allow an attacker to infinitely mint the token as they could -> createBid -> settleCurrentAndCreateNewAuction repeatedly until they have minted as many tokens as they want, while paying reservePrice.
The situation would be contingent on setup, this is additionally attenuated by the fact that Governance would need to vote these irrational values into the contract (defaults are safe), and they would have to do so without simming or running any additional checks.
I think because the lack of POC and detail, I'll downgrade to QA as recommended, however the finding is valid and I recommend the sponsor to check that no overflow can happen nor that a auction can be insta settled under specific circumstances.
I think this was a great opportunity for a more developed report and perhaps next time a coded POC will help push the finding to it's fullest
#5 - GalloDaSballo
2022-09-26T19:41:58Z
L
#6 - iainnash
2022-09-26T20:02:58Z
@GalloDaSballo Good point! Yes with further development and a coded POC this could be a medium risk level bug.
However, in any case still worth providing a fix around better validation here.
#7 - neumoxx
2022-09-27T10:23:41Z
I think the submission has validity and unfortunately is not as developed as it could have.
By thinking about it, if
settings.duration<settings.timeBufferwithtimeBufferbig enough to cause and overflow, then any new auction would be created, and it would be basically ended after starting.Assuming all those values are non-zero then this would allow an attacker to infinitely mint the token as they could -> createBid -> settleCurrentAndCreateNewAuction repeatedly until they have minted as many tokens as they want, while paying
reservePrice.The situation would be contingent on setup, this is additionally attenuated by the fact that Governance would need to vote these irrational values into the contract (defaults are safe), and they would have to do so without simming or running any additional checks.
I think because the lack of POC and detail, I'll downgrade to QA as recommended, however the finding is valid and I recommend the sponsor to check that no overflow can happen nor that a auction can be insta settled under specific circumstances.
I think this was a great opportunity for a more developed report and perhaps next time a coded POC will help push the finding to it's fullest
It's sad because I wanted to express this exact behaviour but just by explining it. Next time I'll sure code a POC, still learning how all this works! Thanks anyway Alex.