Using transferFrom on ERC721 tokens

[code423n4]

Handle

shw

Vulnerability details

Impact

In the function awardExternalERC721 of contract PrizePool, when awarding external ERC721 tokens to the winners, the transferFrom keyword is used instead of safeTransferFrom. If any winner is a contract and is not aware of incoming ERC721 tokens, the sent tokens could be locked.

Proof of Concept

Referenced code:
PrizePool.sol#L602

Recommended Mitigation Steps

Consider changing transferFrom to safeTransferFrom at line 602. However, it could introduce a DoS attack vector if any winner maliciously rejects the received ERC721 tokens to make the others unable to get their awards. Possible mitigations are to use a try/catch statement to handle error cases separately or provide a function for the pool owner to remove malicious winners manually if this happens.

[asselstine]

This issue poses no risk to the Prize Pool, so it's more of a 1 (Low Risk IMO.

This is just about triggering a callback on the ERC721 recipient. We omitted it originally because we didn't want a revert on the callback to DoS the prize pool.

However, to respect the interface it makes sense to implement it fully. That being said, if it does throw we must ignore it to prevent DoS attacks.

[dmvt]

I agree with the medium risk rating provided by the warden.