From c7b9f332f25b92082480427ba555ca9c56870c52 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 14:36:43 -0400 Subject: [PATCH 1/8] fix: incorrect use of library safeApprove for some ERC20 implementations --- contracts/BaseWrapper.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index f8d7c46d..814ab589 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -114,6 +114,7 @@ abstract contract BaseWrapper { } if (token.allowance(address(this), address(_bestVault)) < amount) { + token.safeApprove(address(_bestVault), 0); // Avoid issues with some tokens requiring 0 token.safeApprove(address(_bestVault), UNLIMITED_APPROVAL); // Vaults are trusted } From 6d0e27c8f9f3086e8d01d35b645ce0f4249e5038 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 14:39:35 -0400 Subject: [PATCH 2/8] docs: add note mentioning an assumption of how the Yearn registry works --- contracts/BaseWrapper.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 814ab589..5bef6ea9 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -80,6 +80,9 @@ abstract contract BaseWrapper { } function _updateVaultCache(VaultAPI[] memory vaults) internal { + // NOTE: even though `registry` is update-able by Yearn, the intended behavior + // is that any future upgrades to the registry will replay the version + // history so that this cached value does not get out of date. if (vaults.length > _cachedVaults.length) { _cachedVaults = vaults; } From 915d5d95f01d02e0b0e9f95859e5263a6d017533 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 14:52:10 -0400 Subject: [PATCH 3/8] docs: add note about gas overflow during large withdrawals --- contracts/BaseWrapper.sol | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 5bef6ea9..82f5223f 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -152,6 +152,13 @@ abstract contract BaseWrapper { VaultAPI[] memory vaults = allVaults(); _updateVaultCache(vaults); + // NOTE: This loop will attempt to withdraw from each Vault in `allVaults` that `sender` + // is deposited in, up to `amount` tokens. The withdraw action can be expensive, + // so it if there is a denial of service issue in withdrawing, the downstream usage + // of this wrapper contract must give an alternative method of withdrawing using + // this function so that `amount` is less than the full amount requested to withdraw + // (e.g. "piece-wise withdrawals"), leading to less loop iterations such that the + // DoS issue is mitigated (at a tradeoff of requiring more txns from the end user). for (uint256 id = 0; id < vaults.length; id++) { if (!withdrawFromBest && vaults[id] == _bestVault) { continue; // Don't withdraw from the best From e716ca1780a98e6b9fb8bf663391b01ed87693c9 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 14:57:30 -0400 Subject: [PATCH 4/8] docs: add assumptions for proper usage of constructor args --- contracts/BaseWrapper.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 82f5223f..943e3ff1 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -42,8 +42,9 @@ abstract contract BaseWrapper { uint256 constant UNCAPPED_DEPOSITS = type(uint256).max; constructor(address _token, address _registry) public { + // Recommended to use a token with a `Registry.latestVault(_token) != address(0)` token = IERC20(_token); - // v2.registry.ychad.eth + // Recommended to use `v2.registry.ychad.eth` registry = RegistryAPI(_registry); } From f5bdfb32980c37ef826876410dc241d561f762a3 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 15:09:18 -0400 Subject: [PATCH 5/8] fix: use SafeERC20 functions for transfer --- contracts/BaseWrapper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 943e3ff1..5702baf8 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -139,7 +139,7 @@ abstract contract BaseWrapper { deposited = beforeBal.sub(afterBal); // `receiver` now has shares of `_bestVault` as balance, converted to `token` here // Issue a refund if not everything was deposited - if (depositor != address(this) && afterBal > 0) token.transfer(depositor, afterBal); + if (depositor != address(this) && afterBal > 0) token.safeTransfer(depositor, afterBal); } function _withdraw( From a6f13c9ba5dc3632c7cb28a038c9344a8bc0460a Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 15:15:45 -0400 Subject: [PATCH 6/8] fix: do a check that registry governance doesn't change in setRegistry --- contracts/BaseWrapper.sol | 3 +++ tests/functional/wrappers/conftest.py | 5 +++++ tests/functional/wrappers/test_affliate.py | 22 +++++++++++++++++----- tests/functional/wrappers/test_ytoken.py | 20 ++++++++++++++++---- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 5702baf8..853c65be 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -52,6 +52,9 @@ abstract contract BaseWrapper { require(msg.sender == registry.governance()); // In case you want to override the registry instead of re-deploying registry = RegistryAPI(_registry); + // Make sure there's no change in governance + // NOTE: Also avoid bricking the wrapper from setting a bad registry + require(msg.sender == registry.governance()); } function bestVault() public virtual view returns (VaultAPI) { diff --git a/tests/functional/wrappers/conftest.py b/tests/functional/wrappers/conftest.py index 415e0e25..06eed1cd 100644 --- a/tests/functional/wrappers/conftest.py +++ b/tests/functional/wrappers/conftest.py @@ -34,3 +34,8 @@ def affiliate_token(token, affiliate, registry, AffiliateToken): f"Affiliate {token.symbol()}", f"af{token.symbol()}", ) + + +@pytest.fixture +def new_registry(Registry, gov): + yield gov.deploy(Registry) diff --git a/tests/functional/wrappers/test_affliate.py b/tests/functional/wrappers/test_affliate.py index c945e9b1..75d5f552 100644 --- a/tests/functional/wrappers/test_affliate.py +++ b/tests/functional/wrappers/test_affliate.py @@ -50,15 +50,27 @@ def test_setAffiliate(affiliate, affiliate_token, rando): affiliate_token.acceptAffiliate({"from": affiliate}) -def test_setRegistry(rando, affiliate, gov, affiliate_token): +def test_setRegistry(rando, affiliate, gov, affiliate_token, new_registry): + # Only yGov can call this method with brownie.reverts(): - affiliate_token.setRegistry(rando, {"from": rando}) + affiliate_token.setRegistry(new_registry, {"from": rando}) with brownie.reverts(): - affiliate_token.setRegistry(rando, {"from": affiliate}) + affiliate_token.setRegistry(new_registry, {"from": affiliate}) - # Only yGov can call this method - affiliate_token.setRegistry(rando, {"from": gov}) + # Cannot set to an invalid registry + with brownie.reverts(): + affiliate_token.setRegistry(rando, {"from": gov}) + + # yGov must be the gov on the new registry too + new_registry.setGovernance(rando, {"from": gov}) + new_registry.acceptGovernance({"from": rando}) + with brownie.reverts(): + affiliate_token.setRegistry(new_registry, {"from": gov}) + new_registry.setGovernance(gov, {"from": rando}) + new_registry.acceptGovernance({"from": gov}) + + affiliate_token.setRegistry(new_registry, {"from": gov}) def test_deposit(token, registry, vault, affiliate_token, gov, rando): diff --git a/tests/functional/wrappers/test_ytoken.py b/tests/functional/wrappers/test_ytoken.py index a255f60f..ebe226ba 100644 --- a/tests/functional/wrappers/test_ytoken.py +++ b/tests/functional/wrappers/test_ytoken.py @@ -34,12 +34,24 @@ def test_config(gov, token, vault, registry, ytoken): assert ytoken.allVaults() == [vault] -def test_setRegistry(rando, gov, ytoken): +def test_setRegistry(rando, gov, ytoken, new_registry): + # Only yGov can call this method with brownie.reverts(): - ytoken.setRegistry(rando, {"from": rando}) + ytoken.setRegistry(new_registry, {"from": rando}) - # Only yGov can call this method - ytoken.setRegistry(rando, {"from": gov}) + # Cannot set to an invalid registry + with brownie.reverts(): + ytoken.setRegistry(rando, {"from": gov}) + + # yGov must be the gov on the new registry too + new_registry.setGovernance(rando, {"from": gov}) + new_registry.acceptGovernance({"from": rando}) + with brownie.reverts(): + ytoken.setRegistry(new_registry, {"from": gov}) + new_registry.setGovernance(gov, {"from": rando}) + new_registry.acceptGovernance({"from": gov}) + + ytoken.setRegistry(new_registry, {"from": gov}) def test_deposit(token, registry, vault, ytoken, gov, rando): From a11bcbc5aaa2c3b7fad04f0f75fa9ed3a871b5e7 Mon Sep 17 00:00:00 2001 From: Just some guy <3859395+fubuloubu@users.noreply.github.com> Date: Fri, 2 Apr 2021 15:26:04 -0400 Subject: [PATCH 7/8] fix: add re-entrancy guard on `yWETH.withdrawETH()` --- contracts/yToken.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/yToken.sol b/contracts/yToken.sol index 8e4a39b9..87afeaea 100644 --- a/contracts/yToken.sol +++ b/contracts/yToken.sol @@ -5,6 +5,7 @@ pragma experimental ABIEncoderV2; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {SafeMath} from "@openzeppelin/contracts/math/SafeMath.sol"; +import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; import {VaultAPI, BaseWrapper} from "./BaseWrapper.sol"; @@ -163,7 +164,7 @@ interface IWETH { function withdraw(uint256 wad) external; } -contract yWETH is yToken { +contract yWETH is ReentrancyGuard, yToken { using Address for address payable; constructor(address _weth, address _registry) public yToken(_weth, _registry) {} @@ -177,7 +178,7 @@ contract yWETH is yToken { return _deposit(address(this), msg.sender, amount, false); // `false` = pull from `this` } - function withdrawETH(uint256 amount) external returns (uint256 withdrawn) { + function withdrawETH(uint256 amount) external nonReentrant returns (uint256 withdrawn) { // NOTE: Need to use different method to withdraw than `yToken` withdrawn = _withdraw(msg.sender, address(this), amount, true); // `true` = withdraw from `bestVault` // NOTE: `BaseWrapper.token` is WETH From d285a5d33a78a6859232d99ed2966ab8c091d9cf Mon Sep 17 00:00:00 2001 From: Ser Doggo <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 5 Apr 2021 13:46:07 -0400 Subject: [PATCH 8/8] fix: resolve corner case where `estimatedShares` is <1 --- contracts/BaseWrapper.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/contracts/BaseWrapper.sol b/contracts/BaseWrapper.sol index 853c65be..ac33b1a8 100644 --- a/contracts/BaseWrapper.sol +++ b/contracts/BaseWrapper.sol @@ -193,8 +193,13 @@ abstract contract BaseWrapper { .div(vaults[id].pricePerShare()); // NOTE: Every Vault is different // Limit amount to withdraw to the maximum made available to this contract - uint256 shares = Math.min(estimatedShares, availableShares); - withdrawn = withdrawn.add(vaults[id].withdraw(shares)); + // NOTE: Avoid corner case where `estimatedShares` isn't precise enough + // NOTE: If `0 < estimatedShares < 1` but `availableShares > 1`, this will withdraw more than necessary + if (estimatedShares > 0 && estimatedShares < availableShares) { + withdrawn = withdrawn.add(vaults[id].withdraw(estimatedShares)); + } else { + withdrawn = withdrawn.add(vaults[id].withdraw(availableShares)); + } } else { withdrawn = withdrawn.add(vaults[id].withdraw()); }