MasterVault reserve #141
Merged
MasterVault reserve #141
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
* wip: dead shares and fix div by zero * doc * add one
waelsy123
reviewed
Dec 16, 2025
| } | ||
|
|
||
| function deployVault(address token) public returns (address vault) { | ||
| if (token == address(0)) { |
Contributor
There was a problem hiding this comment.
do we actually need a vault with asset with zero address?
Contributor
Author
There was a problem hiding this comment.
no, i don't think we need to prevent it though
waelsy123
reviewed
Dec 16, 2025
waelsy123
reviewed
Dec 16, 2025
waelsy123
reviewed
Dec 16, 2025
| subVault.withdraw(assetsToWithdraw, address(this), address(this)); | ||
| } | ||
|
|
||
| super._withdraw(caller, receiver, _owner, assets, shares); |
Contributor
Author
There was a problem hiding this comment.
no
function _withdraw(
address caller,
address receiver,
address owner,
uint256 assets,
uint256 shares
) internal virtual {
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);
SafeERC20Upgradeable.safeTransfer(_asset, receiver, assets);
emit Withdraw(caller, receiver, owner, assets, shares);
}
Contributor
|
looks solid implementation to me, we may need a bit of fuzz testing to capture edge cases |
waelsy123
reviewed
Dec 16, 2025
|
|
||
| oldSubVault.redeem(oldSubVault.balanceOf(address(this)), address(this), address(this)); | ||
| IERC20(asset()).safeApprove(address(oldSubVault), 0); | ||
| function rebalance() public { |
Contributor
There was a problem hiding this comment.
since that this method is public, could be exploited by slippage by manipulating sub assets to shares ratio?
Contributor
Author
There was a problem hiding this comment.
hmm, maybe? what would be the exploit?
Sherlock AI |
Comment on lines
+207
to
+228
| function _rebalance() internal { | ||
| uint256 totalAssetsUp = _totalAssetsLessProfit(MathUpgradeable.Rounding.Up); | ||
| uint256 totalAssetsDown = _totalAssetsLessProfit(MathUpgradeable.Rounding.Down); | ||
| uint256 idleTargetUp = totalAssetsUp.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Up); | ||
| uint256 idleTargetDown = totalAssetsDown.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Down); | ||
| uint256 idleBalance = IERC20(asset()).balanceOf(address(this)); | ||
|
|
||
| if (idleTargetDown <= idleBalance && idleBalance <= idleTargetUp) { | ||
| return; | ||
| } | ||
|
|
||
| if (idleBalance < idleTargetDown) { | ||
| // we need to withdraw from subvault | ||
| uint256 assetsToWithdraw = idleTargetDown - idleBalance; | ||
| subVault.withdraw(assetsToWithdraw, address(this), address(this)); | ||
| } | ||
| else { | ||
| // we need to deposit into subvault | ||
| uint256 assetsToDeposit = idleBalance - idleTargetUp; | ||
| subVault.deposit(assetsToDeposit, address(this)); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
Comment on lines
+207
to
+228
| function _rebalance() internal { | ||
| uint256 totalAssetsUp = _totalAssetsLessProfit(MathUpgradeable.Rounding.Up); | ||
| uint256 totalAssetsDown = _totalAssetsLessProfit(MathUpgradeable.Rounding.Down); | ||
| uint256 idleTargetUp = totalAssetsUp.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Up); | ||
| uint256 idleTargetDown = totalAssetsDown.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Down); | ||
| uint256 idleBalance = IERC20(asset()).balanceOf(address(this)); | ||
|
|
||
| if (idleTargetDown <= idleBalance && idleBalance <= idleTargetUp) { | ||
| return; | ||
| } | ||
|
|
||
| if (idleBalance < idleTargetDown) { | ||
| // we need to withdraw from subvault | ||
| uint256 assetsToWithdraw = idleTargetDown - idleBalance; | ||
| subVault.withdraw(assetsToWithdraw, address(this), address(this)); | ||
| } | ||
| else { | ||
| // we need to deposit into subvault | ||
| uint256 assetsToDeposit = idleBalance - idleTargetUp; | ||
| subVault.deposit(assetsToDeposit, address(this)); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
waelsy123
reviewed
Dec 18, 2025
| /// @notice Extra decimals added to the ERC20 decimals of the underlying asset to determine the decimals of the MasterVault | ||
| /// @dev This is done to mitigate the "first depositor" problem described in the OpenZeppelin ERC4626 documentation. | ||
| /// See https://docs.openzeppelin.com/contracts/5.x/erc4626 for more details on the mitigation. | ||
| uint8 public constant EXTRA_DECIMALS = 18; |
Contributor
There was a problem hiding this comment.
inspired by morpho:
DECIMALS_OFFSET = uint8(uint256(18).zeroFloorSub(IERC20Metadata(_asset).decimals()));
where zeroFloorSub is max(0, x-y) = max(0, 18-asset.decimals)
Contributor
Author
There was a problem hiding this comment.
we don't really want an offset of 0, because then the mitigation no longer works
waelsy123
approved these changes
Dec 22, 2025
godzillaba
added a commit
that referenced
this pull request
Dec 23, 2025
* tests: core functionality * handle total supply or total asset zero values * fix mint method when zero totalAssets or totalSupply * perf fees tests and fixes * remove ds store * MasterVault reserve (#141) * wip * wip: conversion and deposit/withdraw functions look okay * settargetAllocationWad * deploy with initial vault * use factory in core test setup * slippage tolerance on setTargetAllocationWad * big simplify * WIP: mastervault donation attack mitigation (#142) * wip: dead shares and fix div by zero * doc * add one * initial approval and small refactor * tests * fix PerformanceFeesWithdrawn event * move event * rebalancing does not count profit * distribute fees before disabling * fix maxMint * fix outdated comment * remove unused errors * nonReentrant * handle max in maxMint * sanity check we have no sub shares when switching * init reentrancy guard * rebalance after setting target * update docs * update outdated comment * fix nonReentrant modifier placement * remove testFoo * fix tests --------- Co-authored-by: Henry <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
testing in progress, but the implementation should be solid unless testing uncovers issues