Platform: Code4rena
Start Date: 24/02/2022
Pot Size: $75,000 USDC
Total HM: 21
Participants: 28
Period: 7 days
Judge: alcueca
Total Solo HM: 15
Id: 94
League: ETH
Rank: 7/28
Findings: 3
Award: $2,827.34
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: gzeon
2434.7549 USDC - $2,434.75
Upgradable escrow contract pose great risk to user who approved their NFT to the contract. Most popular token / NFT exchange do not require user approve their asset to admin upgradable contract.
This also increase user gas usage because they would have to revoke approval when they are done with the protocol.
Separate the escrow contract to make it non-upgradable with a restricted set of functionality.
#0 - HardlyDifficult
2022-03-02T22:17:18Z
This is an interesting suggestion. I'm not sure if the recommendation really reduces the risk though. If we were to make the escrow portion immutable, the trust still depends on the upgradeable market contract to do the correct thing. If I'm understanding the suggestion correctly, the terms of the deal would still be defined in the upgradeable contract - so even with an immutable escrow manager we could theoretically upgrade the market to allow us to buy each of the NFTs for 0 ETH.
🌟 Selected for report: leastwood
Also found by: 0x1f8b, 0xliumin, 0xwags, CertoraInc, Dravee, IllIllI, Ruhum, TerrierLover, WatchPug, cmichel, csanuragjain, defsec, gzeon, hubble, jayjonah8, kenta, kirk-baird, rfa, robee
223.9968 USDC - $224.00
Consider pinning the version of Solidity e.g. =0.8.12
NFTMarketBuyPrice.setBuyPrice allow price=0 which doesn't make much sense to the seller https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketBuyPrice.sol#L150
Considering a high value auction a 10% min increment might be bad for price discovery, might use a tiered model instead
It is too higher consider OS's 2.5% fee
#0 - HardlyDifficult
2022-03-08T20:34:11Z
acceptOffer to revert. I do like the idea of considering a tiered model instead, and we may do that in the future.#1 - alcueca
2022-03-17T09:45:56Z
Unadjusted score: 25 (Including 20 points for the business suggestions)
🌟 Selected for report: Dravee
Also found by: 0x1f8b, CertoraInc, TerrierLover, csanuragjain, gzeon, iain, kenta, rfa, robee, thankthedark
We can use the following function to save gas on float multiplications
// out = x * y unchecked{/} z function fmul(uint256 x, uint256 y, uint256 z) internal pure returns(uint256 out){ assembly{ if iszero(eq(div(mul(x,y),x),y)) {revert(0,0)} out := div(mul(x,y),z) } }
Would save ~60 gas each call which mean > 100 gas in NFTMarketFees._getFees
$ grep -Rn "BASIS_POINTS" contracts | fgrep "*" contracts/mixins/NFTMarketCore.sol:120: uint256 minIncrement = currentAmount * MIN_PERCENT_INCREMENT_IN_BASIS_POINTS; contracts/mixins/NFTMarketFees.sol:194: foundationFee = (price * fee) / BASIS_POINTS; contracts/mixins/NFTMarketFees.sol:202: creatorRev = (price * CREATOR_ROYALTY_BASIS_POINTS) / BASIS_POINTS;
#0 - HardlyDifficult
2022-03-02T22:24:13Z
Ah this is a neat trick.
This is something we'll keep in mind for the future. However given it's fairly small gas savings, I think we'd prefer to not introduce assembly. It could be great if a well trusted & tested library such as OpenZeppelin added this function though.
#1 - alcueca
2022-03-17T15:56:27Z
Score: 200