code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The function initialize of SwappableYieldSource checks that the yield source is valid via _requireYieldSource. When you change the yield source (via swapYieldSource or setYieldSource), then the function _setYieldSource is called. However _setYieldSource doesn't explicitly check the yield source via _requireYieldSource.

The risk is low because there is an indirect check, by the following check, which only succeeds is depositToken is present in the new yield source: require(_newYieldSource.depositToken() == yieldSource.depositToken(), "SwappableYieldSource/different-deposit-token");

For maintenance purposes it is more logical to always call _requireYieldSource, especially if the check would be made more extensive in the future.

Proof of Concept

//https://github.com/pooltogether/swappable-yield-source/blob/main/contracts/SwappableYieldSource.sol#L98 function initialize( IYieldSource _yieldSource, uint8 _decimals, string calldata _symbol, string calldata _name, address _owner) public initializer returns (bool) { _requireYieldSource(_yieldSource);

function _requireYieldSource(IYieldSource _yieldSource) internal view { require(address(_yieldSource) != address(0), "SwappableYieldSource/yieldSource-not-zero-address"); (, bytes memory depositTokenAddressData) = address(_yieldSource).staticcall(abi.encode(_yieldSource.depositToken.selector)); bool isInvalidYieldSource; if (depositTokenAddressData.length > 0) { (address depositTokenAddress) = abi.decode(depositTokenAddressData, (address)); isInvalidYieldSource = depositTokenAddress != address(0); } require(isInvalidYieldSource, "SwappableYieldSource/invalid-yield-source"); }

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

function _requireDifferentYieldSource(IYieldSource _yieldSource) internal view { require(address(_yieldSource) != address(yieldSource), "SwappableYieldSource/same-yield-source"); }

Tools Used

Recommended Mitigation Steps

Add the following statement to _setYieldSource: _requireYieldSource(_newYieldSource);

PierrickGT commented 3 years ago

The _requireYieldSource function is only used to verify that we setup the swappable yield source with an actual yield source.

As noted, we already check that depositToken() exists in the new yield source, so it would be redundant to also add the _requireYieldSource function to perform the same kind of comparaison.

0xean commented 3 years ago

Disagree with sponsor. The current implementation doesn't ensure a fully functional yield source is present.