code423n4 commented 3 years ago

Handle

GalloDaSballo

Vulnerability details

Impact

Detailed description of the impact of this finding.

_requireYieldSource https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L74

Runs a few checks to see if the function depositToken is implemented Notice that this is not a guarantee that the target is a valid Yield Source.

This will simply verify that the contract has that method.

Any malicious attacker could implement that function and then set up the Yield Source to steal funds

In order to guarantee that the target is a valid Yield Source, you'd want to create a registry of know Yield Sources, perhaps controlled by governance or by the DAO, and check against that.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Create any contract with just a function depositToken returns (address) and you'll be able to add pass the check

Tools Used

Recommended Mitigation Steps

Create an on-chain registry of known Yield Sources, either by committee or governance, and use a check against the registry, this will avoid griefing

PierrickGT commented 3 years ago

Swappable Yield Sources will be deployed by a multi sig owned by governance, _requireYieldSource function does indeed simply performs a sanity check to be sure that the yield source address passed is implementing the depositToken function. This is to avoid any human error and deploying a swappable yield source that would be unusable cause the address passed wouldn't be a yield source.

Deployments of a new swappable yield source will be voted by governance, as will a change of yield source, so it would be pretty time and gas consuming to have also to add any new yield source we which to switch to to a registry,

0xean commented 3 years ago

Agree with warden that these checks are not sufficient to deter a malicious implementation. Additionally, switching of the yield source looks to be feasible by the owner (presumably the above mentioned multisig) or the AssetManager which is unclear who controls this address. Leaving open.