setPrizeStrategy check for Interface Supported in PrizePool.sol doesn't guarantee that the new prize strategy is valid

[code423n4]

Handle

GalloDaSballo

Vulnerability details

Impact

The check in https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L1007

Seems to imply that a strategy is valid as long as it implements the ERC165_INTERFACE_ID_TOKEN_LISTENER.

However, any attacker could fabricate a contract that supports the interface, while still draining or locking funds.

Based on who the owner can be (whether it's the creator of the prize or governance), this can be riskier or less risky

If governance is timelocked, then the risk of setting the wrong strategy is minimal.

If the creator of the pool can be a random address, then this is effectively allowing them to attach any strategy that will steal funds without any tangible protection.

Proof of Concept

I could just copy the PrizeStrategy code, then change the deposit function to steal all funds / interest

Recommended Mitigation Steps

Checking for a property in the contract is not a security guarantee.

I would recommend having a record of all valid PrizeStrategies, controller by governance, to ensure that the only strategies that can be set are vetted by the community / governance.

[asselstine]

First of all there is no deposit function on the prize strategy; the prize strategy only has access to the interest earned on the principal. The prize strategy can't rug the pool.

We explicitly want to allow people to swap strategies. The risk is mitigated to the interest, so it's low risk for depositors.

[dmvt]

This assumes that the owner is a bad actor. If we agree that owners as bad actors constitutes a risk, than nearly every protocol of this nature has the risk. Users should be aware that the owner is slightly trusted, so I'm going to relabel this as informational / non-critical and let it stand, but I agree with the sponsor that no action needs to be taken.