Enforce ERC4626 previews on wrapper

contracts/BIFI/vaults/BeefyWrapper.sol

@@ -2,7 +2,8 @@
 
 pragma solidity ^0.8.0;
 
-import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
+import {ERC4626Upgradeable, ERC20Upgradeable, MathUpgradeable, IERC20MetadataUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
+import {SafeERC20Upgradeable, IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
 
 /**
  * @dev Interface of a Beefy Vault
@@ -23,12 +24,25 @@ interface IVault {
  * @notice Implementation for an ERC-4626 wrapper of a Beefy Vault
  * @dev Wrapped Beefy Vault tokens can be minted by deposit of the underlying asset or by 
  * wrapping Beefy Vault tokens in a 1:1 ratio. Wrapped Beefy Vault tokens can either be unwrapped
- * for an equal number of Beefy Vault tokens or redeemed for the underlying asset
+ * for an equal number of Beefy Vault tokens or redeemed for the underlying asset.
+ * ERC4626 rules are strictly enforced, preview functions should return the correct values.
+ * Only vaults which do not update their asset balance on deposit can be wrapped, i.e. vaults which
+ * have profit locked or not harvesting on deposit, and the underlying balance is not updated on interactions.
  */
 contract BeefyWrapper is ERC4626Upgradeable {
     using SafeERC20Upgradeable for IERC20Upgradeable;
     using MathUpgradeable for uint256;
 
+    /**
+     * @notice Error for when the shares are not minted correctly
+     */
+    error MissingShares();
+
+    /**
+     * @notice Error for when the assets are not transferred correctly
+     */
+    error LeftOverAssets();
+
     /**
      * @notice Address of the vault being wrapped
      */
@@ -45,7 +59,7 @@ contract BeefyWrapper is ERC4626Upgradeable {
         address _vault,
         string memory _name,
         string memory _symbol
-    ) public initializer {
+    ) external initializer {
         vault = _vault;
         __ERC20_init(_name, _symbol);
         __ERC4626_init(IVault(vault).want());
@@ -90,7 +104,7 @@ contract BeefyWrapper is ERC4626Upgradeable {
      * @dev Returns the total assets held by the vault, not only the wrapper
      * @return totalAssets the total balance of assets held by the vault
      */
-    function totalAssets() public view virtual override returns (uint256) {
+    function totalAssets() public view override returns (uint256) {
         return IVault(vault).balance();
     }
 
@@ -100,7 +114,7 @@ contract BeefyWrapper is ERC4626Upgradeable {
      * @return totalSupply the total supply of vault shares
      */
     function totalSupply()
-        public view virtual override(ERC20Upgradeable, IERC20Upgradeable) 
+        public view override(ERC20Upgradeable, IERC20Upgradeable) 
     returns (uint256) {
         return IERC20Upgradeable(vault).totalSupply();
     }
@@ -119,14 +133,15 @@ contract BeefyWrapper is ERC4626Upgradeable {
         address receiver,
         uint256 assets,
         uint256 shares
-    ) internal virtual override {
-        IERC20Upgradeable(asset()).safeTransferFrom(caller, address(this), assets);
-        uint balance = IERC20Upgradeable(vault).balanceOf(address(this));
+    ) internal override {
+        super._deposit(caller, receiver, assets, shares);
+
+        uint256 balance = IERC20Upgradeable(vault).balanceOf(address(this));
+
         IVault(vault).deposit(assets);
-        shares = IERC20Upgradeable(vault).balanceOf(address(this)) - balance;
-        _mint(receiver, shares);
 
-        emit Deposit(caller, receiver, assets, shares);
+        /// Prevent harvest on deposit vaults from under-minting to the wrapper
+        if (shares != IERC20Upgradeable(vault).balanceOf(address(this)) - balance) revert MissingShares();
     }
 
     /**
@@ -145,20 +160,34 @@ contract BeefyWrapper is ERC4626Upgradeable {
         address owner,
         uint256 assets,
         uint256 shares
-    ) internal virtual override {
-        if (caller != owner) {
-            _spendAllowance(owner, caller, shares);
-        }
-        _burn(owner, shares);
+    ) internal override {
+        uint256 balance = IERC20Upgradeable(asset()).balanceOf(address(this));
 
         IVault(vault).withdraw(shares);
-        uint balance = IERC20Upgradeable(asset()).balanceOf(address(this));
-        if (assets > balance) {
-            assets = balance;
-        }
 
-        IERC20Upgradeable(asset()).safeTransfer(receiver, assets);
+        super._withdraw(caller, receiver, owner, assets, shares);
 
-        emit Withdraw(caller, receiver, owner, assets, shares);
+        /// Prevent assets from being left over in the wrapper
+        if (IERC20Upgradeable(asset()).balanceOf(address(this)) != balance) revert LeftOverAssets();
+    }
+
+    /**
+     * @dev Internal conversion function (from assets to shares) with support for rounding direction, reverting back to pre-v4.9.0 behavior.
+     * @param assets the amount of assets to be converted to shares
+     * @param rounding the rounding direction
+     * @return shares the amount of shares that corresponds to the assets
+     */
+    function _convertToShares(uint256 assets, MathUpgradeable.Rounding rounding) internal view override returns (uint256) {
+        return assets.mulDiv(totalSupply(), totalAssets(), rounding);
+    }
+
+    /**
+     * @dev Internal conversion function (from shares to assets) with support for rounding direction, reverting back to pre-v4.9.0 behavior.
+     * @param shares the amount of shares to be converted to assets
+     * @param rounding the rounding direction
+     * @return assets the amount of assets that corresponds to the shares
+     */
+    function _convertToAssets(uint256 shares, MathUpgradeable.Rounding rounding) internal view override returns (uint256) {
+        return shares.mulDiv(totalAssets(), totalSupply(), rounding);
     }
 }

forge/test/interfaces/IStrategy.sol

@@ -16,6 +16,8 @@ interface IStrategy {
     event Unpaused(address account);
     event Withdraw(uint256 tvl);
 
+    error StrategyPaused();
+
     // All strats
 
     function MAX_CALL_FEE() external view returns (uint256);

forge/test/wrapper/WrapperTest.t.sol

@@ -14,20 +14,20 @@ import {IERC20Like} from "../interfaces/IERC20Like.sol";
 
 // Users
 import {WrapperUser} from "../users/WrapperUser.sol";
+import {BeefyWrapperFactory} from "../../../contracts/BIFI/vaults/BeefyWrapperFactory.sol";
 
 contract WrapperTest is BaseTestHarness {
 
     // Input your vault to test here.
-    IWrapperFactory factory = IWrapperFactory(0x48bF3a071098a09C7D00379b4DBC69Ab6Da83a36); 
-    IVault constant vault = IVault(0xf6a1284Dc2ce247Bca885ac4F36b37E91d3bD032); // Moo Hop ETH on Arbitrum
+    IVault constant vault = IVault(0xdb6E5dC4C6748EcECb97b565F6C074f24384fD07); // Moo Silo USDC on Sonic
     IWrapper wrapper;
     IStrategy strategy;
 
     // Users
     WrapperUser user;
     address constant keeper = 0x4fED5491693007f0CD49f4614FFC38Ab6A04B619;
-    address constant vaultOwner = 0x9A94784264AaAE397441c1e47fA132BE4e61BdaD;
-    address constant strategyOwner = 0x6d28afD25a1FBC5409B1BeFFf6AEfEEe2902D89F;
+    address constant vaultOwner = 0xC35a456138dE0634357eb47Ba5E74AFE9faE9a98;
+    address constant strategyOwner = 0x73F97432Adb2a1c39d0E1a6e554c7d4BbDaFC316;
 
     IERC20Like want;
     uint256 slot; // Storage slot that holds `balanceOf` mapping.
@@ -38,6 +38,7 @@ contract WrapperTest is BaseTestHarness {
 
 
     function setUp() public {
+        BeefyWrapperFactory factory = new BeefyWrapperFactory();
         wrapper = IWrapper(factory.clone(address(vault)));
 
         want = IERC20Like(wrapper.asset());
@@ -71,7 +72,6 @@ contract WrapperTest is BaseTestHarness {
         console.log("Final user want balance", wantBalanceFinal);
         assertGt(shares, shareEstimate * 99 / 100, "Minted shares > estimated shares * 99 / 100");
         assertGt(wantBalanceFinal, withdrawEstimate * 99 / 100, "Expected wantBalanceFinal > withdrawEstimate * 99 / 100");
-        assertLe(wantBalanceFinal, wantStartingAmount, "Expected wantBalanceFinal <= wantStartingAmount");
         assertGt(wantBalanceFinal, wantStartingAmount * 99 / 100, "Expected wantBalanceFinal > wantStartingAmount * 99 / 100");
     }
 
@@ -109,10 +109,6 @@ contract WrapperTest is BaseTestHarness {
         uint256 timestampBeforeHarvest = block.timestamp;
         shift(delay);
 
-        console.log("Testing call rewards > 0");
-        uint256 callRewards = strategy.callReward();
-        assertGt(callRewards, 0, "Expected callRewards > 0");
-
         console.log("Harvesting vault.");
         bool didHarvest = _harvest();
         assertTrue(didHarvest, "Harvest failed.");
@@ -162,7 +158,7 @@ contract WrapperTest is BaseTestHarness {
 
         // Users can't deposit.
         console.log("Trying to deposit while panicked.");
-        FORGE_VM.expectRevert("Pausable: paused");
+        FORGE_VM.expectRevert(IStrategy.StrategyPaused.selector);
         user.depositAll(wrapper);
 
         // User can still withdraw
@@ -222,6 +218,29 @@ contract WrapperTest is BaseTestHarness {
         }
     }
 
+    function test_rounding_error() external {
+        uint256 assets = wrapper.previewMint(1 ether);
+
+        user.approve(address(want), address(wrapper), assets);
+        uint256 userWantBalanceBeforeDeposit = want.balanceOf(address(user));
+        user.mint(wrapper, 1 ether);
+
+        uint256 shares = wrapper.balanceOf(address(user));
+        assertEq(shares, 1 ether, "Shares should be equal to the preview");
+
+        assets = wrapper.previewRedeem(shares);
+        userWantBalanceBeforeDeposit = want.balanceOf(address(user));
+        user.redeem(wrapper, shares);
+
+        assertEq(assets, want.balanceOf(address(user)) - userWantBalanceBeforeDeposit, "Assets should be equal to the preview");
+
+        shares = wrapper.previewDeposit(1 ether);
+        user.approve(address(want), address(wrapper), 1 ether);
+        user.deposit(wrapper, 1 ether);
+
+        assertEq(shares, wrapper.balanceOf(address(user)), "Shares should be equal to the preview");
+    }
+
     /*         */
     /* Helpers */
     /*         */

package.json

@@ -59,7 +59,7 @@
     "abi-to-sol": "abi-to-sol",
     "installForge": ". scripts/forge/installForge.sh",
     "forgeTest:vault": "forge test --force --fork-url http://localhost:8545 --match-contract ProdVaultTest",
-    "forgeTest:wrapper": "forge test --force --fork-url https://arb1.arbitrum.io/rpc --match-contract WrapperTest",
+    "forgeTest:wrapper": "forge test --force --fork-url https://sonic-rpc.publicnode.com --match-contract WrapperTest",
     "forgeTest:oracle": "forge test --force --fork-url https://polygon-rpc.com --match-contract Oracle",
     "forgeTest:swapper": "forge test --force --fork-url https://polygon-rpc.com --match-contract Swapper",
     "forgeTest:aura": "forge test --force --fork-url https://rpc.ankr.com/eth --match-contract StrategyAuraGyroTest",