search

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

0 stars 0 forks source link

SwappableYieldSource: Missing same deposit token check in transferFunds() #29

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

hickuphh3

Vulnerability details

Impact

transferFunds() will transfer funds from a specified yield source _yieldSource to the current yield source set in the contract _currentYieldSource. However, it fails to check that the deposit tokens are the same. If the specified yield source's assets are of a higher valuation, then a malicious owner or asset manager will be able to exploit and pocket the difference.

Proof of Concept

Assumptions:

Attacker does the following:

  1. Deposit 100 DAI into the swappable yield source contract
  2. Call transferFunds(_yieldSource, 100 * 1e18)
    • _requireDifferentYieldSource() passes
    • _transferFunds(_yieldSource, 100 * 1e18) is called
      • _yieldSource.redeemToken(_amount); → This will transfer 100 WETH out of the _yieldSource into the contract
      • uint256 currentBalance = IERC20Upgradeable(_yieldSource.depositToken()).balanceOf(address(this)); → This will equate to ≥ 100 WETH.
      • require(_amount <= currentBalance, "SwappableYieldSource/transfer-amount-different"); is true since both are 100 * 1e18
      • _currentYieldSource.supplyTokenTo(currentBalance, address(this)); → This supplies the transferred 100 DAI from step 1 to the current yield source
    • We now have 100 WETH in the swappable yield source contract
  3. Call transferERC20(WETH, attackerAddress, 100 * 1e18) to withdraw 100 WETH out of the contract to the attacker's desired address.

Recommended Mitigation Steps

_requireDifferentYieldSource() should also verify that the yield sources' deposit token addresses are the same.

function _requireDifferentYieldSource(IYieldSource _yieldSource) internal view {
    require(address(_yieldSource) != address(yieldSource), "SwappableYieldSource/same-yield-source");
        require(_newYieldSource.depositToken() == yieldSource.depositToken(), "SwappableYieldSource/different-deposit-token");
}
PierrickGT commented 3 years ago

This exploit was indeed possible when we had the transferFunds function but now that we have removed it and funds can only be moved by swapYieldSource(), this exploit is no longer possible since we check for the same depositToken in _setYieldSource().

https://github.com/pooltogether/swappable-yield-source/pull/4

0xean commented 3 years ago

Upgrading to 3 considering the potential for loss of funds