Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix deposit fee #38

Merged
merged 6 commits into from
Sep 12, 2023
Merged

Fix deposit fee #38

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f624290
feat: update test naming
MerlinEgalite Sep 7, 2023
4c7b5d6
Merge pull request #22 from morpho-labs/feat/timelock
MerlinEgalite Sep 7, 2023
fb95a81
fix(vault): sync last total assets last
Rubilmax Sep 12, 2023
17e69bc
Merge branch 'test/deposit-withdraw-tests' of github.com:morpho-labs/…
Rubilmax Sep 12, 2023
fc73b4c
Merge branch 'test/fees' of github.com:morpho-labs/morpho-blue-metamo…
Rubilmax Sep 12, 2023
f058644
refactor(vault): rename sync to update
Rubilmax Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 67 additions & 38 deletions contracts/SupplyVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
Pending public pendingTimelock;
uint256 public timelock;

/// @dev Stores the total assets owned by this vault when the fee was last accrued.
/// @dev Stores the total assets this vault manage when it last handled a deposit/withdraw.
uint256 public lastTotalAssets;
uint256 public lastUpdateTimestamp;

ConfigSet private _config;

Expand Down Expand Up @@ -98,6 +97,12 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
_;
}

modifier syncLastTotalAssets() {
Rubilmax marked this conversation as resolved.
Show resolved Hide resolved
_;

lastTotalAssets = totalAssets();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we trigger 2 times totalAssets for each tx. It will cost a lot...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it is the only viable solution. Using a timestamp-based solution creates a bug: if 2 users deposit within the same block, lastTotalAssets is not updated with the last user's deposit, which gets counted as interest in the next fee accrual...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed an alternative version that saves 2 calls to totalAssets. This should be a huge gas cost improvement

}

/* ONLY OWNER FUNCTIONS */

function submitPendingTimelock(uint256 newTimelock) external onlyOwner {
Expand Down Expand Up @@ -139,7 +144,7 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
emit EventsLib.SubmitPendingFee(newFee);
}

function setFee() external timelockElapsed(pendingFee.timestamp) onlyOwner {
function setFee() external timelockElapsed(pendingFee.timestamp) onlyOwner syncLastTotalAssets {
// Accrue interest using the previous fee set before changing it.
_accrueFee();

Expand All @@ -150,7 +155,7 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
delete pendingFee;
}

function setFeeRecipient(address newFeeRecipient) external onlyOwner {
function setFeeRecipient(address newFeeRecipient) external onlyOwner syncLastTotalAssets {
require(newFeeRecipient != feeRecipient, ErrorsLib.ALREADY_SET);

// Accrue interest to the previous fee recipient set before changing it.
Expand Down Expand Up @@ -231,28 +236,24 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
return _market(id).cap;
}

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

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

return _staticWithdrawOrder(super.maxWithdraw(owner));
}

function maxRedeem(address owner) public view override returns (uint256) {
return _convertToShares(maxWithdraw(owner), Math.Rounding.Down);
}

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

return super.deposit(assets, receiver);
function deposit(uint256 assets, address receiver) public virtual override returns (uint256 shares) {
shares = _convertToSharesWithFeeAccrued(assets, Math.Rounding.Down);
_deposit(_msgSender(), receiver, assets, shares);
}

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

return super.mint(shares, receiver);
function mint(uint256 shares, address receiver) public virtual override returns (uint256 assets) {
assets = _convertToAssetsWithFeeAccrued(shares, Math.Rounding.Up);
_deposit(_msgSender(), receiver, assets, shares);
}

function withdraw(uint256 assets, address receiver, address owner)
Expand All @@ -261,20 +262,16 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
override
returns (uint256 shares)
{
_accrueFee();

// Do not call expensive `maxWithdraw` and optimistically withdraw assets.

shares = previewWithdraw(assets);
shares = _convertToSharesWithFeeAccrued(assets, Math.Rounding.Up);
_withdraw(_msgSender(), receiver, owner, assets, shares);
}

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

// Do not call expensive `maxRedeem` and optimistically redeem shares.

assets = previewRedeem(shares);
assets = _convertToAssetsWithFeeAccrued(shares, Math.Rounding.Down);
_withdraw(_msgSender(), receiver, owner, assets, shares);
}

Expand All @@ -290,8 +287,14 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
assets += ERC20(asset()).balanceOf(address(this));
}

/* ERC4626 (INTERNAL) */

/// @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 {
function _deposit(address caller, address owner, uint256 assets, uint256 shares)
internal
override
syncLastTotalAssets
{
super._deposit(caller, owner, assets, shares);

require(_depositOrder(assets) == 0, ErrorsLib.DEPOSIT_ORDER_FAILED);
Expand All @@ -301,12 +304,45 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares)
internal
override
syncLastTotalAssets
{
require(_withdrawOrder(assets) == 0, ErrorsLib.WITHDRAW_ORDER_FAILED);

super._withdraw(caller, receiver, owner, assets, shares);
}

function _convertToShares(uint256 assets, Math.Rounding rounding)
internal
view
virtual
override
returns (uint256)
{
return assets.mulDiv(totalSupply() + _accruedFeeShares() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
}

function _convertToAssets(uint256 shares, Math.Rounding rounding)
internal
view
virtual
override
returns (uint256)
{
return shares.mulDiv(totalAssets() + 1, totalSupply() + _accruedFeeShares() + 10 ** _decimalsOffset(), rounding);
}

function _convertToSharesWithFeeAccrued(uint256 assets, Math.Rounding rounding) internal returns (uint256) {
_accrueFee();

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

function _convertToAssetsWithFeeAccrued(uint256 shares, Math.Rounding rounding) internal returns (uint256) {
_accrueFee();

return shares.mulDiv(totalAssets() + 1, totalSupply() + 10 ** _decimalsOffset(), rounding);
}

/* INTERNAL */

function _market(Id id) internal view returns (VaultMarket storage) {
Expand Down Expand Up @@ -457,30 +493,23 @@ contract SupplyVault is ERC4626, Ownable2Step, ISupplyVault {
}

function _accrueFee() internal {
uint256 lastUpdate = lastUpdateTimestamp;
lastUpdateTimestamp = block.timestamp;

if (lastUpdate == block.timestamp || fee == 0 || feeRecipient == address(0)) return;
if (fee == 0 || feeRecipient == address(0)) return;

(uint256 newTotalAssets, uint256 feeShares) = _accruedFeeShares();

lastTotalAssets = newTotalAssets;
uint256 feeShares = _accruedFeeShares();

if (feeShares != 0) _mint(feeRecipient, feeShares);

emit EventsLib.AccrueFee(newTotalAssets, feeShares);
}

function _accruedFeeShares() internal view returns (uint256 newTotalAssets, uint256 feeShares) {
newTotalAssets = totalAssets();
function _accruedFeeShares() internal view returns (uint256 feeShares) {
uint256 newTotalAssets = totalAssets();
uint256 totalInterest = newTotalAssets.zeroFloorSub(lastTotalAssets);

if (totalInterest != 0) {
uint256 feeAmount = totalInterest.mulDiv(fee, WAD);
// The fee amount is subtracted from the total assets in this calculation to compensate for the fact
// that total assets is already increased by the total interest (including the fee amount).
feeShares = feeAmount.mulDiv(
totalSupply() + 10 ** _decimalsOffset(), newTotalAssets - feeAmount + 1, Math.Rounding.Down
uint256 feeAssets = totalInterest.mulDiv(fee, WAD);
// The fee assets is subtracted from the total assets in this calculation to compensate for the fact
// that total assets is already increased by the total interest (including the fee assets).
feeShares = feeAssets.mulDiv(
totalSupply() + 10 ** _decimalsOffset(), newTotalAssets - feeAssets + 1, Math.Rounding.Down
);
}
}
Expand Down
5 changes: 0 additions & 5 deletions contracts/libraries/EventsLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,4 @@ library EventsLib {
event SetCap(uint128 cap);

event DisableMarket(Id id);

/// @notice Emitted when the vault's performance fee is accrued.
/// @param totalAssets The total amount of assets this vault manages.
/// @param feeShares The shares minted corresponding to the fee accrued.
event AccrueFee(uint256 totalAssets, uint256 feeShares);
}
3 changes: 1 addition & 2 deletions contracts/mocks/IrmMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ contract IrmMock is IIrm {
function borrowRateView(MarketParams memory, Market memory market) public view returns (uint256) {
uint256 utilization = market.totalBorrowAssets.wDivDown(market.totalSupplyAssets);

// Divide by the number of seconds in a year.
// This is a very simple model where x% utilization corresponds to x% APR.
// When rate is zero, x% utilization corresponds to x% APR.
return rate == 0 ? utilization / 365 days : rate / 365 days;
}

Expand Down
Loading