code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Ticket.sol#L12

Vulnerability details

Impact

ERC721 NFT collections have to implement "Ownable" to be properly customizable at NFT marketplaces like Opensea (more details below). While the Lottery.sol contract (which inherits the Ticket.sol ERC721 contract) implements "Ownable" (via inheritance of "RNSourceController.sol"), there is a conflict between two security level requirements for the owner wallet.

The Owner of the RNSourceController can customize the source of the Randomness (via e.g. "swapSource"), a very critical part of the system => should at least be secured via a MultiSig, better even with a timelocked governance contract in between
The Owner of a ERC721 collection typically can do nothing but storefront customization at NFT marketplaces and the config of royalties (which is way less critical). This is only possible with EOA accounts today and not with multisigs (https://support.opensea.io/hc/en-us/articles/1500007978402-What-crypto-wallets-can-I-use-with-OpenSea-) => should be implemented via EOA account (multisig not possible, hardware wallet recommended)

Because of these two different security requirements it is imo critical that the ERC721 ticket contract owner and the RNSourceController owner are separated.

Details why "ownable" is so important for ERC721:

The customization on most large marketplaces relies on the ERC721 contract implementing "Ownable". The owner address of the contract is then qualified to make changes to the collection storefront at e.g. Opensea. This is e.g. described at the Opensea Docs (https://docs.opensea.io/docs/8-customizing-your-storefront: "you'll need to make sure your contract is Ownable;"). And while the WenWin lottery contract (and therefore also the ticket ERC721) implements ownable, it should be implemented via Multisig, which doesn't work with e.g. OpenSea.

I don't see any chance that major secondary sales of the tickets will occur without proper customization of the ticket ERC721 collection at NFT Marketplaces like Opensea. And not being able to verify and connect the marketplace storefront to e.g. WenWin`s twitter will open up a lot of space for fake collections of tickets.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

To test that there is a owner of the lottery contract (and therefore ticket contract), add the following test to Lottery.t.sol and run "forge test --match testCallOwnerOnLotter" :

" function testCallOwnerOnLotter() public { lottery.owner(); } "

Tools Used

Forge Test

Recommended Mitigation Steps

A possible solution should maintain the current contract inheritance structure but still separate ownership of "Ticket.sol" and "RNSourceController.sol". A very straightforward way to do that is to make "Ticket.sol" "Ownable" and to use the role based "AccessControl" contract from Openzepplin (https://docs.openzeppelin.com/contracts/4.x/api/access#AccessControl) for the RNSourceController.sol contract.

The only necessary changes would be

Ticket.sol: import "@openzeppelin/contracts/access/Ownable2Step.sol";
Ticket.sol: abstract contract Ticket is ITicket, ERC721, Ownable2Step {...
RNSourceController.sol: replace Ownable2Step import with "@openzeppelin/contracts/access/AccessControl.sol"
RNSourceController.sol: abstract contract RNSourceController is AccessControl, ....
RNSourceController.sol: replace "onlyOwner" with "onlyRole(DEFAULT_ADMIN_ROLE)"
RNSourceController.sol: add "_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);" to constructor
add "function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC721, AccessControl) returns (bool) { return super.supportsInterface(interfaceId); }" to "Lottery.sol"

c4-judge commented 1 year ago

thereksfour changed the severity to QA (Quality Assurance)