PoolTogether micro contest #1 - GalloDaSballo's results

Handle

GalloDaSballo

Vulnerability details

Impact

The function swapYieldSource https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L307

Can be called by the owner (deployer / initializer) or Asset Manager

The function will take all funds from the old Yield Source, and transfer them to the new Yield source.

Any contract that implement the function function depositToken() external returns (address) will pass the check

However, if either the owner or the assetManager have malicious intent, this function allows them to instantly rug all funds

Proof of Concept

1. Create a contract that implements the function depositToken() external returns (address)
2. Be the Owner or AssetManager
3. Call setYieldSource while pointing at your malicious contract
4. Profit

Recommended Mitigation Steps

I highly recommend checking that the YieldSource is from a trusted registry before allowing this swap.

Alternatively forcing each Owner to be a TimeLock with at least 48 hours may provide enough security to allow this to be used in practice

Handle

GalloDaSballo

Vulnerability details

Impact

When swapping from one yield source to another, via the function _setYieldSource https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L254

You are correctly giving approval to the new yield source. However, since you are retiring the old one, it's probably best to remove the allowance from the old yield source

Recommended Mitigation Steps

Change

  function _setYieldSource(IYieldSource _newYieldSource) internal {
    _requireDifferentYieldSource(_newYieldSource);
    require(_newYieldSource.depositToken() == yieldSource.depositToken(), "SwappableYieldSource/different-deposit-token");

    yieldSource = _newYieldSource;
    IERC20Upgradeable(_newYieldSource.depositToken()).safeApprove(address(_newYieldSource), type(uint256).max);

    emit SwappableYieldSourceSet(_newYieldSource);
  }

To

  function _setYieldSource(IYieldSource _newYieldSource) internal {
    _requireDifferentYieldSource(_newYieldSource);
    require(_newYieldSource.depositToken() == yieldSource.depositToken(), "SwappableYieldSource/different-deposit-token");
        IERC20Upgradeable(yieldSource.depositToken()).safeApprove(address(_newYieldSource), 0);
    yieldSource = _newYieldSource;
    IERC20Upgradeable(_newYieldSource.depositToken()).safeApprove(address(_newYieldSource), type(uint256).max);

    emit SwappableYieldSourceSet(_newYieldSource);
  }