Skip to content

Commit

Permalink
Merge pull request #63 from morpho-labs/fix/withdraw-error
Browse files Browse the repository at this point in the history 
refactor(vault): rewrite _deposit & _withdraw
  • Loading branch information
@Rubilmax
Rubilmax committed Sep 15, 2023
2 parents ceb32a0 + d9f3891 commit c79ad79
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 60 deletions.
103 changes: 54 additions & 49 deletions src/MetaMorpho.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {IMorphoMarketParams} from "./interfaces/IMorphoMarketParams.sol";
import {MarketAllocation, Pending, IMetaMorpho} from "./interfaces/IMetaMorpho.sol";
import {Id, MarketParams, Market, IMorpho} from "@morpho-blue/interfaces/IMorpho.sol";

import "src/libraries/ConstantsLib.sol";
import {ErrorsLib} from "./libraries/ErrorsLib.sol";
import {EventsLib} from "./libraries/EventsLib.sol";
import {WAD} from "@morpho-blue/libraries/MathLib.sol";
Expand All @@ -30,12 +31,6 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
using MarketParamsLib for MarketParams;
using MorphoBalancesLib for IMorpho;

/* CONSTANTS */

uint256 public constant TIMELOCK_EXPIRATION = 2 days;
uint256 public constant MAX_TIMELOCK = 2 weeks;
uint256 public constant DECIMALS_OFFSET = 6;

/* IMMUTABMES */

IMorpho internal immutable MORPHO;
Expand Down Expand Up @@ -231,9 +226,9 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
return _market(id).cap;
}

/* ERC4626 */
/* ERC4626 (PUBLIC) */

function maxWithdraw(address owner) public view virtual override returns (uint256) {
function maxWithdraw(address owner) public view override returns (uint256) {
_accruedFeeShares();

return _staticWithdrawOrder(super.maxWithdraw(owner));
Expand All @@ -243,7 +238,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
return _convertToShares(maxWithdraw(owner), Math.Rounding.Down);
}

function deposit(uint256 assets, address receiver) public virtual override returns (uint256 shares) {
function deposit(uint256 assets, address receiver) public override returns (uint256 shares) {
uint256 newTotalAssets = _accrueFee();

shares = _convertToSharesWithFeeAccrued(assets, newTotalAssets, Math.Rounding.Down);
Expand All @@ -252,7 +247,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
_updateLastTotalAssets(newTotalAssets + assets);
}

function mint(uint256 shares, address receiver) public virtual override returns (uint256 assets) {
function mint(uint256 shares, address receiver) public override returns (uint256 assets) {
uint256 newTotalAssets = _accrueFee();

assets = _convertToAssetsWithFeeAccrued(shares, newTotalAssets, Math.Rounding.Up);
Expand All @@ -261,12 +256,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
_updateLastTotalAssets(newTotalAssets + assets);
}

function withdraw(uint256 assets, address receiver, address owner)
public
virtual
override
returns (uint256 shares)
{
function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256 shares) {
uint256 newTotalAssets = _accrueFee();

// Do not call expensive `maxWithdraw` and optimistically withdraw assets.
Expand All @@ -277,7 +267,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
_updateLastTotalAssets(newTotalAssets - assets);
}

function redeem(uint256 shares, address receiver, address owner) public virtual override returns (uint256 assets) {
function redeem(uint256 shares, address receiver, address owner) public override returns (uint256 assets) {
uint256 newTotalAssets = _accrueFee();

// Do not call expensive `maxRedeem` and optimistically redeem shares.
Expand All @@ -300,42 +290,19 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
assets += ERC20(asset()).balanceOf(address(this));
}

/// @dev Used in mint or deposit to deposit the underlying asset to Blue markets.
function _deposit(address caller, address owner, uint256 assets, uint256 shares) internal override {
super._deposit(caller, owner, assets, shares);

require(_depositOrder(assets) == 0, ErrorsLib.DEPOSIT_ORDER_FAILED);
}
/* ERC4626 (INTERNAL) */

/// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets.
function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares)
internal
override
{
require(_withdrawOrder(assets) == 0, ErrorsLib.WITHDRAW_ORDER_FAILED);

super._withdraw(caller, receiver, owner, assets, shares);
function _decimalsOffset() internal pure override returns (uint8) {
return 6;
}

function _convertToShares(uint256 assets, Math.Rounding rounding)
internal
view
virtual
override
returns (uint256)
{
function _convertToShares(uint256 assets, Math.Rounding rounding) internal view override returns (uint256) {
(uint256 feeShares, uint256 newTotalAssets) = _accruedFeeShares();

return assets.mulDiv(totalSupply() + feeShares + 10 ** _decimalsOffset(), newTotalAssets + 1, rounding);
}

function _convertToAssets(uint256 shares, Math.Rounding rounding)
internal
view
virtual
override
returns (uint256)
{
function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view override returns (uint256) {
(uint256 feeShares, uint256 newTotalAssets) = _accruedFeeShares();

return shares.mulDiv(newTotalAssets + 1, totalSupply() + feeShares + 10 ** _decimalsOffset(), rounding);
Expand All @@ -357,6 +324,48 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
return shares.mulDiv(newTotalAssets + 1, totalSupply() + 10 ** _decimalsOffset(), rounding);
}

/// @dev Used in mint or deposit to deposit the underlying asset to Blue markets.
function _deposit(address caller, address owner, uint256 assets, uint256 shares) internal override {
// If asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
// `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
// calls the vault, which is assumed not malicious.
//
// Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
// assets are transferred and before the shares are minted, which is a valid state.
// slither-disable-next-line reentrancy-no-eth
SafeERC20.safeTransferFrom(IERC20(asset()), caller, address(this), assets);

require(_depositOrder(assets) == 0, ErrorsLib.DEPOSIT_ORDER_FAILED);

_mint(owner, shares);

emit Deposit(caller, owner, assets, shares);
}

/// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets.
function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares)
internal
override
{
if (caller != owner) {
_spendAllowance(owner, caller, shares);
}

// If asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
// `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
// calls the vault, which is assumed not malicious.
//
// Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
// shares are burned and after the assets are transferred, which is a valid state.
_burn(owner, shares);

require(_withdrawOrder(assets) == 0, ErrorsLib.WITHDRAW_ORDER_FAILED);

SafeERC20.safeTransfer(IERC20(asset()), receiver, assets);

emit Withdraw(caller, receiver, owner, assets, shares);
}

/* INTERNAL */

function _market(Id id) internal view returns (VaultMarket storage) {
Expand Down Expand Up @@ -532,8 +541,4 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
);
}
}

function _decimalsOffset() internal pure override returns (uint8) {
return 6;
}
}
12 changes: 12 additions & 0 deletions src/libraries/ConstantsLib.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.0;

/// @dev The delay after a timelock ends after which the owner must submit a parameter again.
/// It guarantees users that the owner only accepts parameters submitted recently.
uint256 constant TIMELOCK_EXPIRATION = 2 days;

/// @dev The maximum delay of a timelock.
uint256 constant MAX_TIMELOCK = 2 weeks;

/// @dev OpenZeppelin's decimals offset used in MetaMorpho's ERC4626 implementation.
uint256 constant DECIMALS_OFFSET = 6;
10 changes: 6 additions & 4 deletions test/forge/ERC4626Test.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,17 +167,19 @@ contract ERC4626Test is BaseTest {
vault.transferFrom(ONBEHALF, RECEIVER, shares);
}

function testWithdrawTooMuch(uint256 deposited) public {
function testWithdrawTooMuch(uint256 deposited, uint256 assets) public {
deposited = bound(deposited, MIN_TEST_ASSETS, MAX_TEST_ASSETS);

borrowableToken.setBalance(SUPPLIER, deposited);

vm.prank(SUPPLIER);
vault.deposit(deposited, ONBEHALF);

vm.prank(SUPPLIER);
vm.expectRevert(bytes(ErrorsLib.WITHDRAW_ORDER_FAILED));
vault.withdraw(deposited + 1, RECEIVER, ONBEHALF);
assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET));

vm.prank(ONBEHALF);
vm.expectRevert("ERC20: burn amount exceeds balance");
vault.withdraw(assets, RECEIVER, ONBEHALF);
}

function testTransfer(uint256 deposited, uint256 toTransfer) public {
Expand Down
6 changes: 3 additions & 3 deletions test/forge/MarketTest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract MarketTest is BaseTest {
}

function testEnableMarketShouldRevertWhenTimelockNotElapsed(uint128 timelock, uint256 timeElapsed) public {
timelock = uint128(bound(timelock, 1, vault.MAX_TIMELOCK()));
timelock = uint128(bound(timelock, 1, MAX_TIMELOCK));
_submitAndSetTimelock(timelock);

timeElapsed = bound(timeElapsed, 0, timelock - 1);
Expand All @@ -53,10 +53,10 @@ contract MarketTest is BaseTest {
}

function testEnableMarketShouldRevertWhenTimelockExpirationExceeded(uint128 timelock, uint256 timeElapsed) public {
timelock = uint128(bound(timelock, 1, vault.MAX_TIMELOCK()));
timelock = uint128(bound(timelock, 1, MAX_TIMELOCK));
_submitAndSetTimelock(timelock);

timeElapsed = bound(timeElapsed, timelock + vault.TIMELOCK_EXPIRATION() + 1, type(uint128).max);
timeElapsed = bound(timeElapsed, timelock + TIMELOCK_EXPIRATION + 1, type(uint128).max);

vm.startPrank(RISK_MANAGER);
vault.submitMarket(allMarkets[0], CAP);
Expand Down
6 changes: 3 additions & 3 deletions test/forge/TimelockTest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import "./helpers/BaseTest.sol";

contract TimelockTest is BaseTest {
function testSubmitPendingTimelock(uint256 timelock) public {
timelock = bound(timelock, 0, vault.MAX_TIMELOCK());
timelock = bound(timelock, 0, MAX_TIMELOCK);
vm.prank(OWNER);
vault.submitTimelock(timelock);

Expand All @@ -20,15 +20,15 @@ contract TimelockTest is BaseTest {
}

function testSubmitPendingTimelockShouldRevertWhenMaxTimelockExceeded(uint256 timelock) public {
timelock = bound(timelock, vault.MAX_TIMELOCK() + 1, type(uint256).max);
timelock = bound(timelock, MAX_TIMELOCK + 1, type(uint256).max);

vm.prank(OWNER);
vm.expectRevert(bytes(ErrorsLib.MAX_TIMELOCK_EXCEEDED));
vault.submitTimelock(timelock);
}

function testSetTimelock(uint256 timelock) public {
timelock = bound(timelock, 0, vault.MAX_TIMELOCK());
timelock = bound(timelock, 0, MAX_TIMELOCK);
vm.startPrank(OWNER);
vault.submitTimelock(timelock);

Expand Down
3 changes: 2 additions & 1 deletion test/forge/helpers/BaseTest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {MarketParamsLib} from "@morpho-blue/libraries/MarketParamsLib.sol";
import {MorphoLib} from "@morpho-blue/libraries/periphery/MorphoLib.sol";
import {MorphoBalancesLib} from "@morpho-blue/libraries/periphery/MorphoBalancesLib.sol";

import {ORACLE_PRICE_SCALE} from "@morpho-blue/libraries/ConstantsLib.sol";
import "src/libraries/ConstantsLib.sol";
import "@morpho-blue/libraries/ConstantsLib.sol";

import {IrmMock} from "src/mocks/IrmMock.sol";
import {ERC20Mock} from "src/mocks/ERC20Mock.sol";
Expand Down

0 comments on commit c79ad79

Please sign in to comment.