review contracts #1
Conversation
There was a problem hiding this comment.
👍 👁️ 👄 👁️ 👍
Looks good! Lots of suggestions but no real vulns. People should expect to spend everything they put into this contract with conventional auction mechanics, but that probably doesn't cancel out the value prop. Any IMarketWrapper contract will have to be heavily scrutinized.
| // NFT contract | ||
| address public nftContract; | ||
| // Fractionalized NFT vault responsible for post-auction value capture | ||
| address public tokenVault; |
There was a problem hiding this comment.
Nit
| address public tokenVault; | |
| ITokenVault public tokenVault; |
There was a problem hiding this comment.
wow for some reason I was under the impression that if I stored contracts cast to their interface, it messed up querying the address off-chain. but i just wrote some tests and that's not the case. facepalm moment
| // market auctioning the NFT | ||
| address public marketWrapper; | ||
| // NFT contract | ||
| address public nftContract; |
There was a problem hiding this comment.
Nit:
| address public nftContract; | |
| IERC721Metadata public nftContract; |
|
|
||
| // market wrapper contract exposing interface for | ||
| // market auctioning the NFT | ||
| address public marketWrapper; |
There was a problem hiding this comment.
Nit:
| address public marketWrapper; | |
| IMarketWrapper public marketWrapper; |
| // ============ Immutables ============ | ||
|
|
||
| address public immutable partyDAOMultisig; | ||
| address public immutable tokenVaultFactory; |
There was a problem hiding this comment.
Nit:
| address public immutable tokenVaultFactory; | |
| IERC721VaultFactory public immutable tokenVaultFactory; |
|
|
||
| address public immutable partyDAOMultisig; | ||
| address public immutable tokenVaultFactory; | ||
| address public immutable weth; |
There was a problem hiding this comment.
Nit:
| address public immutable weth; | |
| IWETH public immutable weth; |
| if (partyStatus == PartyStatus.AUCTION_WON) { | ||
| // transfer ETH fee to PartyDAO | ||
| _ethFee = _getEthFee(highestBid); | ||
| _transferETHOrWETH(partyDAOMultisig, _ethFee); |
There was a problem hiding this comment.
partyDAOMultisig could technically reenter into one of the admin functions here and mess with things but I guess they can already do a rug pull in an easier manner atm. But if you do decide to curb their power, maybe consider putting a reentrancy guard on the admin functions too.
| { | ||
| require(partyStatus != PartyStatus.AUCTION_ACTIVE, "PartyBid::totalEthUsedForBid: party still active; amounts undetermined"); | ||
| // get all of the contributor's contributions | ||
| Contribution[] memory _contributions = contributions[_contributor]; |
There was a problem hiding this comment.
This loads the entire array into memory. You can potentially save a lot of gas if you mark this storage (and cache _contributions.length) if people make multiple contributions and the break gets hit. On the other hand, if that won't be the case, this is probably slightly cheaper.
| // after the auction has been finalized, | ||
| // if the NFT is owned by the PartyBid, then the PartyBid won the auction | ||
| address _owner = _getOwner(); | ||
| partyStatus = _owner == address(this) ? PartyStatus.AUCTION_WON : PartyStatus.AUCTION_LOST; |
There was a problem hiding this comment.
If someone else wins the auction but transfers the 721 to this contract before finalize() is called, does highestBid ETH just get stuck in here? Kind of a pointless grief though.
There was a problem hiding this comment.
yes that would be a possible grief, but in that scenario, it would just behave as if the users won the NFT. everyone would still receive fractional tokens and the contract would generally behave as if the purpose that the users initially wanted (winning the NFT) was acheived.
but yes, the ETH they didn't actually spend would be locked in the contract as if it was spent
| * callable by anyone (doesn't have to be the contributor) | ||
| * @param _contributor the address of the contributor | ||
| */ | ||
| function claim(address _contributor) external nonReentrant { |
There was a problem hiding this comment.
I am pretty confident that you can get rid of all these reentrancy guards with some slight reordering of operations, but that could easily change as features get added.
There was a problem hiding this comment.
ditto
yeah the thinking behind the re-entrancy guards was really just to be overly defensive and not have to think about the possibility of re-entrancy
| ) | ||
| ); | ||
|
|
||
| deployedAt[partyBidProxy] = block.number; |
There was a problem hiding this comment.
What's the point of this?
There was a problem hiding this comment.
it allows folks on-chain or off-chain to query for a PartyBid address to see if it was deployed by the canonical Factory. the choice to record block number is somewhat arbitrary
Based on https://github.com/PartyDAO/partybid/tree/2b1fb4349e9ada1df713362b17c4ca9628560bd2
Scope:
InitializedProxy.solPartyBid.solPartyBidFactory.sol