Cabin Passport Roast

roast.md

General

• Try to use latest version of Solidity (0.8.12 at time of writing) for compiler optimizations, features, and bugfixes

Minor security:

• Consider using a TwoStepOwnable implementation to mitigate the risk of unrecoverable ownership transfers due to user error

Minor optimizations:

• Consider using custom errors instead of require statements, which can cut down on contract size and thus deploy gas cost
• For token optimizations: consider inheriting from Solmate's (opinionated) token contracts over OpenZeppelin's

Contracts

Passport.sol

• L16 - price can be immutable if there is no setter method (should there be?)
• L17 - rather than tracking remaining supply, I recommend tracking both maxSupply and a mintIndex to track the maximum ID that has already been minted
• L18 - metadataHash sounds like it should be used in place of baseURI when calculating tokenURI but it is not surfaced that way; likewise: _baseURI() is not overidden
• L19 - isPrivate doesn't seem to be updatable outside of the constructor – do you want the contract to be Pausable?
• L22 - claimable see comment about isPrivate
• L23 - Consider relying on the default Transfer events from the ERC721 Spec. Unclear what specific benefit the Purchase event provides.
• L97 - buy method should be nonReentrant. If comments from L17 are addressed, does not need to take an _id param.
• L97 - Consider only accepting exact payment
• L106-114 - Consider adding a withdraw method and adding all payment split logic there so the gas cost of transfers is not passed onto the buyer. Also consider using Solmate's SafeTransfer library, which is gas-optimized.
• L121 - consider pre-decrement operator --supply for gas savings (~60-80 gas)
• L126 - this function and its arguments seem unnecessary. Public properties like price and supply are redundant
• L153 - Consider solmate's SafeTransfer library and moving payment splitter logic here.
• L165 - Unless some external service is listening for Received events from this contract, emitting an event is unnecessary
• L173 - Consider using a single merkleRoot and using Merkle Proofs to verify an address is a minter. This would also allow the contract to inherit the much simpler Ownable contract
• L200 - Consider pre-increment operator ++i
• L245 - Minor performance improvement: cache accounts.length as a variable before the loop uint256 accountsLength = accounts.length; and reference that in the loop. Also consider pre-increment operator ++i
• L252 - Unclear what benefit this event serves if not tracked by an external service

PassportFactory.sol

Broadly

• Consider combining both Passport and PassportFactory into a single ERC-1155 contract (though this will complicate royalty splits)
• Consider making PassportFactory an ERC721 token itself, and delegating ownership/authorization checks in individual Passports to the PassportFactory's ownerOf method

Line-by-line

• L57-64 - This method does no authorization checks and does not check that passport is a Passport
• L68 - There is no withdraw method, meaning all ether sent to this contract will be lost
• L69 - Emitting a Received event is likely unnecessary

Staking.sol

Broadly

• A public method to get total staked balance might be useful
• This contract seems unfinished, as there is no benefit to staking, unless other contracts rely on isStakeHolder(address) to return true, but even then, without a way to tell the amount a person has staked, it may not be useful information.

Line-by-line

• L9 - SafeMath is unnecessary
• L11 - indices is not used aside from being updated when staking+unstaking. Consider removing.
• L22 - Consider using Solmate's SafeTransferLib when transferring ERC20 tokens
• L28 - StakeCreated event is not useful to front-end hooks without identifying information, like the address that created the stake
• L35 - Any address that is not a stakeholder can call removeStake with 0 as an argument, which will delete the first address from the stakeholders array.
• L49 - see L22
• L50 - see L28