Forgeries contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Admin can turn rogue and steal the winner's NFT

Proof of Concept

Compromised admin can call lastResortTimelockOwnerClaimNFT to steal the winner's NFT.

function lastResortTimelockOwnerClaimNFT() external onlyOwner {
	// If recoverTimelock is not setup, or if not yet occurred
	if (settings.recoverTimelock > block.timestamp) {
		// Stop the withdraw
		revert RECOVERY_IS_NOT_YET_POSSIBLE();
	}

	// Send event for indexing that the owner reclaimed the NFT
	emit OwnerReclaimedNFT(owner());

	// Transfer token to the admin/owner.
	IERC721EnumerableUpgradeable(settings.token).transferFrom(
		address(this),
		owner(),
		settings.tokenId
	);
}

the NFT is transferred to owner(), not the winner. If the owner just do not want to give the winner's NFT and turn rogue, winner is rugged.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project change the lastResortTimelockOwnerClaimNFT to:

/// @notice Optional last resort admin reclaim nft function
/// @dev Only callable by the owner
function lastResortTimelockOwnerClaimNFT() external onlyOwner {
	// If recoverTimelock is not setup, or if not yet occurred
	if (settings.recoverTimelock > block.timestamp) {
		// Stop the withdraw
		revert RECOVERY_IS_NOT_YET_POSSIBLE();
	}
	
	// here
	address winner = IERC721EnumerableUpgradeable(settings.drawingToken).ownerOf(
      request.currentChosenTokenId
  );
  
	// Send event for indexing that the owner reclaimed the NFT
	emit OwnerReclaimedNFT(winner);

	// Transfer token to the admin/owner.
	IERC721EnumerableUpgradeable(settings.token).safeTransferFrom(
		address(this),
		winner,
		settings.tokenId
	);
}

can use safeTransfer to make sure the receiver can receive NFT. If the safeTransfer revert, then transfer the NFT to owner.