Skip to content
40 changes: 27 additions & 13 deletions contracts/tokenbridge/libraries/vault/MasterVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@
error NewSubVaultExchangeRateTooLow();
error BeneficiaryNotSet();
error PerformanceFeeDisabled();
error NoSharesRedeemed();
error NoSubvaultShares();
error NoSharesBurned();
error InvalidAsset();
error InvalidOwner();

// todo: avoid inflation, rounding, other common 4626 vulns
// we may need a minimum asset or master share amount when setting subvaults (bc of exchange rate calc)
Expand All @@ -54,8 +59,8 @@
event BeneficiaryUpdated(address indexed oldBeneficiary, address indexed newBeneficiary);

function vaultInit(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, for convention

Suggested change
function vaultInit(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {
function initialize(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {

require(address(_asset) != address(0), "INVALID_ASSET");
require(_owner != address(0), "INVALID_OWNER");
if (address(_asset) == address(0)) revert InvalidAsset();
if (_owner == address(0)) revert InvalidOwner();

__ERC20_init(_name, _symbol);
__ERC4626_init(IERC20Upgradeable(address(_asset)));
Expand Down Expand Up @@ -104,36 +109,42 @@
_revokeSubVault(minAssetExchRateWad);
}

function _setSubVault(IERC4626 _subVault, uint256 minSubVaultExchRateWad) internal {
if (address(_subVault) == address(0)) revert SubVaultCannotBeZeroAddress();
if (totalSupply() == 0) revert MustHaveSupplyBeforeSettingSubVault();
if (address(_subVault.asset()) != address(asset())) revert SubVaultAssetMismatch();

uint256 _totalAssets = totalAssets();
uint256 _totalSupply = totalSupply();

subVault = _subVault;

IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);
uint256 subShares = _subVault.deposit(totalAssets(), address(this));
uint256 subShares = _subVault.deposit(_totalAssets, address(this));

uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), MathUpgradeable.Rounding.Down);
uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
if (_subVaultExchRateWad < minSubVaultExchRateWad) revert SubVaultExchangeRateTooLow();
subVaultExchRateWad = _subVaultExchRateWad;

subVault = _subVault;

emit SubvaultChanged(address(0), address(_subVault));
}

Check warning

Code scanning / Slither

Dangerous strict equalities Medium


function _revokeSubVault(uint256 minAssetExchRateWad) internal {
IERC4626 oldSubVault = subVault;
if (address(oldSubVault) == address(0)) revert NoExistingSubVault();

uint256 _totalSupply = totalSupply();
uint256 assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)), address(this), address(this));
uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived();
uint256 maxWithdrawAmount = oldSubVault.maxWithdraw(address(this));

IERC20(asset()).safeApprove(address(oldSubVault), 0);
subVault = IERC4626(address(0));
subVaultExchRateWad = 1e18;

uint256 assetReceived = oldSubVault.withdraw(maxWithdrawAmount, address(this), address(this));
IERC20(asset()).safeApprove(address(oldSubVault), 0);

uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived();

emit SubvaultChanged(address(oldSubVault), address(0));
}

Expand Down Expand Up @@ -182,7 +193,8 @@
if (totalProfits > 0) {
IERC4626 _subVault = subVault;
if (address(_subVault) != address(0)) {
_subVault.withdraw(totalProfits, address(this), address(this));
uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));
if (sharesRedeemed == 0) revert NoSharesRedeemed();
Copy link
Contributor

Choose a reason for hiding this comment

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

let's leave this as is since it's not relevant to upgradeability

Suggested change
uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));
if (sharesRedeemed == 0) revert NoSharesRedeemed();
_subVault.withdraw(totalProfits, address(this), address(this));

}
IERC20(asset()).safeTransfer(beneficiary, totalProfits);
}
Expand Down Expand Up @@ -260,7 +272,8 @@
totalPrincipal += assets;
IERC4626 _subVault = subVault;
if (address(_subVault) != address(0)) {
_subVault.deposit(assets, address(this));
uint256 subShares = _subVault.deposit(assets, address(this));
if (subShares == 0) revert NoSubvaultShares();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the rationale behind this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just to satisfy slither as there is a rule to not have unused variables / unused returns

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i think we can ignore slither

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've reverted these changes to cover slither with different PR

}
}

Expand All @@ -278,7 +291,8 @@

IERC4626 _subVault = subVault;
if (address(_subVault) != address(0)) {
_subVault.withdraw(assets, address(this), address(this));
uint256 sharesBurned = _subVault.withdraw(assets, address(this), address(this));
if (sharesBurned == 0) revert NoSharesBurned();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@godzillaba same here

}

super._withdraw(caller, receiver, _owner, assets, shares);
Expand Down
Loading