Platform: Code4rena
Start Date: 11/08/2022
Pot Size: $40,000 USDC
Total HM: 8
Participants: 108
Period: 4 days
Judge: hickuphh3
Total Solo HM: 2
Id: 152
League: ETH
Rank: 1/108
Findings: 5
Award: $9,387.62
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: Lambda
6341.8225 USDC - $6,341.82
royaltyInfo, getRoyalties, or getFeeRecipients may return address(0) as the recipient address. While the value 0 is correctly handled for the royalties itself, it is not for the address. In such a case, the ETH amount will be sent to address(0), i.e. it is burned and lost.
In your logic for determining the recipients, treat address(0) as if no recipient was returned such that the other priorities / methods take over.
#0 - HardlyDifficult
2022-08-19T11:35:48Z
We are looking into options here to improve.
We believe this is Medium risk, burning a percent of the sale revenue is a form of leaking value. Otherwise the sale works as expected and the collector does get the NFT they purchased.
The royalty APIs we use are meant to specific which addresses should receive payments and how much they each should receive. As the warden noted, we try to ignore entries which specify a 0 amount... but did not filter out address(0) recipients with >0 requested. Originally we were thinking this was a way of requesting that a portion of the sale be burned since that seems to be what the data is proposing.
However we agree that this is more likely a configuration error. Since our market uses ETH and not ERC20 tokens, it's unlikely that creators would actually want a portion of the proceeds burned. We are exploring a change to send the additional revenue to the seller instead of burning the funds in this scenario.
#1 - HickupHH3
2022-08-25T07:37:27Z
Case of protocol leaked value: medium severity is appropriate.
🌟 Selected for report: KIntern_NA
Also found by: Lambda
https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L244 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L302
If royaltyInfo returns 0 for the NFT contract, this is ignored. On the other hand, if 0 is returned for the override contract, the returned address gets 100% of the royalties. This should be unified to either always give 100% of the royalties in such a case or never do so. The latter option would be more sensible in my opinion, as the standard states: "Marketplaces that support this standard SHOULD NOT send a zero-value transaction if the royaltyAmount returned is 0. This would waste gas and serves no useful purpose in this EIP."
Meaning no transfer is expected when zero is returned.
Unify the business logic with regards to ERC-2981
#0 - HardlyDifficult
2022-08-18T18:55:45Z
🌟 Selected for report: Lambda
Also found by: 0x52, KIntern_NA
1155.7971 USDC - $1,155.80
If one creator specified a share that is larger than BASIS_POINTS, the first creator gets all of the royalties. Depending on how these are set (which is not in the control of the project), this can be exploited by the first creator.
A collective of artists have implemented a process where everyone can set its own share of the fees to the value that he thinks is fair and these values are returned by getRoyalties. Bob, the first entry in the list of _recipients sets its value to a value that is slightly larger than BASIS_POINTS such that he gets all of the royalties.
There is no need for this check / logic, as the whole sum (totalShares) is used anyway to normalize the values.
#0 - HardlyDifficult
2022-08-17T20:49:28Z
ACK.
We believe this is low risk. For the Foundation collections, the royalty rate is hard coded to 10% or via PercentSplitETH which is not subject to this issue. For 3rd party collections, there are more direct ways to change the distribution if the creator was attempting to be malicious towards their partners -- esp via the Royalty Registry.
This report is true. And the recommendation seems reasonable. However we will not be making this change. We are currently investigating changing our royalty logic in order to use the values returned by collections directly, instead of normalizing it to 10% like we do now. Most of the royalty APIs used here are not official standards, but are becoming industry standards based on growing adoption -- and they are expecting the percent amounts to be defined in Basis Points.
We do not want to mislead the community too much to ease the pain of the potential upcoming change I mention above. If they are returning values > 10,000 we don't want that pattern to be adopted by more collections.
Another option may be to ignore the results if totalShares sums to > 10,000 - that's tempting but we are going to defer making a change like that until a future workstream which will be more dedicated to rethinking royalties.
#1 - HickupHH3
2022-08-27T01:07:11Z
Am siding with the warden here, because for 3rd party collections, it may be the case that they use a larger denomination than basis points. As mentioned in a different issue, royalty standards are still in its infancy.
Most of the royalty APIs used here are not official standards, but are becoming industry standards based on growing adoption -- and they are expecting the percent amounts to be defined in Basis Points.
Hopeful for this to be the case so there is less ambiguity, and non-compliance can be ignored as suggested by the sponsor.
🌟 Selected for report: rbserver
Also found by: 0xc0ffEE, CodingNameKiki, Deivitto, Diraco, IllIllI, KIntern_NA, Lambda, Noah3o6, Treasure-Seeker, ignacio, oyc_109, zeesaw
NFTDropCollection currently uses _mint instead of _safeMint. This is disencouraged, because onERC721Received is not called if the recipient is a smart contract. Therefore, when the recipient does not want to receive the NFT or has some custom business logic (e.g., internal accounting) on receive, this will be ignored.
Replace _mint with _safeMint. However, simply doing this would actually introduce a reentrancy vulnerability in NFTDropMarketFixedPriceSale.mintFromFixedPriceSale that would allow users to mint more than the limitPerAccount. Therefore, this function also needs a reentrancy guard in this case.
#0 - HardlyDifficult
2022-08-18T12:18:29Z
🌟 Selected for report: Saw-mon_and_Natalie
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xSmartContract, 0xSolus, 0xackermann, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, Chom, Deivitto, DevABDee, Dravee, ElKu, IllIllI, JC, Kumpa, Lambda, LeoS, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Treasure-Seeker, Vexjon, Waze, Yiko, __141345__, apostle0x01, auditor0517, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, c3phas, carlitox477, cccz, cryptphi, csanuragjain, d3e4, danb, delfin454000, durianSausage, erictee, fatherOfBlocks, gogo, iamwhitelights, joestakey, jonatascm, ladboy233, mics, oyc_109, rbserver, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, simon135, thank_you, wagmi, yash90, zeesaw, zkhorse
164.5359 USDC - $164.54
__gap differs per contract, some use 1000 uints (NFTDropMarketCore), other use 2000 (FoundationTreasuryNode). Consider using a unified size. Furthermore, using a smaller value such as 50 (what OpenZeppelin does) should be sufficient.getFeesAndRecipients, the referral info is not included (https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L193)_getFees, it is mentioned that "If the seller is unknown, assume it's being sold by the creator." (https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L462). However, this is actually not implemented. While this is not too bad as the seller is currently always known, it is a confusing comment.buyReferrer is not equal to msg.sender (https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L524). Of course, this check can easily be circumvented by providing a different address to always get a part of the fees.#0 - HardlyDifficult
2022-08-18T21:23:11Z
The size of the __gap differs per contract
The inconsistencies here are due to changes we've had with contracts in the past. Large gaps are used to leave room for new mixins in the future, e.g. adding an OZ dependency may require 50 slots so we need more than that available in ours to remain flexible.
Unresolved TODO comment
Agree, will fix.
If the seller is unknown, assume it's being sold by the creator
Good catch, this was leftover from some now since removed code.
this check can easily be circumvented
Understood - see https://github.com/code-423n4/2022-08-foundation-findings/issues/134