code-423n4 / 2023-03-wenwin-findings

1 stars 1 forks source link

Not implementing a _baseURI method #105

Closed code423n4 closed 1 year ago

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

The ticket contract doesn't implement a _baseURI method. Therefore all tokenURIs return empty strings. This makes trading of the ERC721 tickets on secondary marketplaces like Opensea nearly impossible for users and there severely impacts the function of the whole protocol. The WenWin docs explicitly state that tickets should be tradeable on secondary: https://docs.wenwin.com/wenwin-lottery/the-game/tickets

Proof of Concept

The _baseURI method from the imported ERC721 (OpenZepplin) does only return an empty string (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/0457042d93d9dfd760dbaa06a4d2f1216fdbe297/contracts/token/ERC721/ERC721.sol#L105)

To prove that this behaviour is inherited by Ticket.sol and therefore Lottery.sol you can add the following tests to "Lottery.t.sol". Run them with "forge test --match testProofOfEmptyTokenURI" and "forge test --match testTokenURIForNonExistentTicket":

" // test that shows that the ticket ERC721 returns empty tokenURIs for existing tickets function testProofOfEmptyTokenURI() public { testBuyTicket();

// proof that id 0 exists
address owner_0 = lottery.ownerOf(0);
assertEq(owner_0, USER);

// proof that id 0 has empty tokenURI
string memory tokenUri_0 = lottery.tokenURI(0);
assertEq(tokenUri_0, "");

// proof that id 1 has empty tokenURI
string memory tokenUri_1 = lottery.tokenURI(0);
assertEq(tokenUri_1, "");

}

// showing that in case of non-existent tickets the tokenURI method doesn't return an empty string but reverts function testFailTokenURIForNonExistentTicket() public { testBuyTicket();

// proof that it fails for non-existent tickets
string memory tokenUri_1000 = lottery.tokenURI(1000);
assertEq(tokenUri_1000, "");

} "

Tools Used

forge test

Recommended Mitigation Steps

Solution 1: Implement a "_baseURI" method in the "Ticket.sol" contract that returns a string state variable pointing to an API endpoint that returns metadata in the correct format. The tokenURI method of the OpenZepplin ERC721 automatically attaches the queried id to the end of that API url. A guide can be found here https://docs.opensea.io/docs/metadata-standards. I would recommend to also integrate a ownable-protected setter for the string state variable. Downside of this solution is that you have to assemble the metadata offchain, which requires calls to the contract (for the 7/35 numbers of a ticket) or event processing, which is both rather effortful.

Better solution (imo): Generating the metadata completely onchain, since most the most important data (the 7/35 numbers for the ticket) are stored onchain anyway. This saves you the problems to operate an API endpoint (which would be centralized, since ipfs is not applicable here due to dynamic metadata). A beautiful (but a lot more complex) example of a tokenURI method that assembles json onchain is the checks.art ChecksMetadata.sol contract (https://etherscan.io/address/0x036721e5a769cc48b3189efbb9cce4471e8a48b1#code#F16#L29). Happy to help here if you want support with the implementation of an onchain metadata solution.

c4-judge commented 1 year ago

thereksfour marked the issue as primary issue

c4-sponsor commented 1 year ago

TutaRicky marked the issue as disagree with severity

TutaRicky commented 1 year ago

This issue is not for 2 (Med Risk)

c4-judge commented 1 year ago

thereksfour changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

thereksfour marked the issue as grade-b