search

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

0 stars 0 forks source link

SwappableYieldSource. _setYieldSource is not removing approval for the old Yield Source #13

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

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);
  }
PierrickGT commented 3 years ago

Duplicate of https://github.com/code-423n4/2021-07-pooltogether-findings/issues/3