search

code-423n4 / 2021-06-pooltogether-findings

0 stars 0 forks source link

Using `transferFrom` on ERC721 tokens #115

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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