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

1 stars 1 forks source link

Ticket minting should use `safeMint` #431

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Ticket.sol#L23-L27

Vulnerability details

Lottery tickets are represented by NFTs using the ERC721 standard. When tickets are bought, the protocol will mint the corresponding NFT to the caller using the _mint internal call to the OpenZeppelin implementation. This function doesn't implement the check to ensure the caller is able to handle ERC721 NFTs in case it is a contract.

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Ticket.sol#L23-L27

function mint(address to, uint128 drawId, uint120 combination) internal returns (uint256 ticketId) {
    ticketId = nextTicketId++;
    ticketsInfo[ticketId] = TicketInfo(drawId, combination, false);
    _mint(to, ticketId);
}

Impact

Contracts that interact with the protocol and purchase tickets will likely result in the loss of tickets if the caller contract is unaware of the NFT transfer and fails to implement proper support to handle them.

Recommendation

Use _safeMint when minting tickets:

function mint(address to, uint128 drawId, uint120 combination) internal returns (uint256 ticketId) {
    ticketId = nextTicketId++;
    ticketsInfo[ticketId] = TicketInfo(drawId, combination, false);
    _safeMint(to, ticketId);
}
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-a

c4-sponsor commented 1 year ago

TutaRicky marked the issue as sponsor disputed

TutaRicky commented 1 year ago

We are aware of this, but we dispute validity because of gas and potential reentrancy (and because EOA from the frontend will mostly use lottery).

thereksfour commented 1 year ago

see https://github.com/code-423n4/2023-03-wenwin-findings/issues/488#issuecomment-1473736157