Forgeries contest - carrotsmuggler's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L304-L320

Vulnerability details

Impact

The contract isn't designed to be used multiple times. However, under certain conditions the admin can start and stop the lottery at will, denying some winners the prize if they chose to do so. This can be done using lastResortTimelockOwnerClaimNFT() and redraw() functions. This is problematic since the contract can be used outside of its designed purpose as shown in the POC.

Proof of Concept

Assume the conditions to call lastResortTimelockOwnerClaimNFT() are satisfied. This can be easily done by creating the lottery and waiting until the time specified in settings.recoverTimelock. This is also satisfied if the lottery simply has been going on for a long time (users not claiming, winning token is burnt/not minted, etc)

1. Run function reroll() as admin
2. Wait for winner (VRF callback)
3. Admin doesn't like winner, so admin calls lastResortTimelockOwnerClaimNFT() to remove the NFT
4. Waits settings.drawBufferTime seconds. Then transfers NFT back to lottery with transferFrom and calls reroll()
5. reroll() passes all checks since the lottery is the owner of the NFT again. Rerolls the winner denying the last winner

Similar behavior can also be observed after removing NFT with winnerClaimNFT(), transferring it and reroll()

Tools Used

Manual Review

Recommended Mitigation Steps

Consider destroying the contract once done. Implement a function

function _end () internal {
    // event End();
    emit End();
    selfdestruct(owner);    
}

Call this function after the NFT is transferred out either via lastResortTimelockOwnerClaimNFT() or via winnerClaimNFT()