fix: mixbytes wrapper audit

contracts/BaseWrapper.sol

@@ -42,15 +42,19 @@ 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);
     }
 
     function setRegistry(address _registry) external {
         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) {
@@ -80,6 +84,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;
         }
@@ -114,6 +121,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
         }
 
@@ -134,7 +142,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(
@@ -148,6 +156,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

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

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)

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):

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):