Conversation
…reator contract size
gzeoneth
left a comment
There was a problem hiding this comment.
still think the fee toggle just add complexity without good usecase
| address masterVaultRoles, | ||
| address masterVaultBeaconProxyFactory |
There was a problem hiding this comment.
breaks CREATE2-like deterministic deployment behavior as these new param are not part of _getL1Salt and _getL2Salt; we should consider to deploy these contract in the factory instead of taking untrusted input
| * @title Layer 1 Gateway contract for bridging standard ERC20s with YBB enabled | ||
| * @notice Escrows funds into MasterVaults for yield bearing bridging. | ||
| */ | ||
| contract L1YbbERC20Gateway is L1ERC20Gateway { |
There was a problem hiding this comment.
L2 token created with underlying token decimals, but MV share have +6 decimals and L2 token represent MV share instead of underlying so we need to adjust somewhere
| /// - Set the rebalance cooldown | ||
| bytes32 public constant GENERAL_MANAGER_ROLE = keccak256("GENERAL_MANAGER_ROLE"); | ||
| /// @notice The fee manager can: | ||
| /// - Toggle performance fees on/off |
There was a problem hiding this comment.
Consider to take this away from FEE_MANAGER_ROLE because the current implementation allow reseting the high water mark by toggling the fee off and on. If the fee manager can temporally manipulate sub-vault price it can drain the vault too.
| .mulDiv(1e18, totalSupply(), MathUpgradeable.Rounding.Up) | ||
| ); | ||
| } else { | ||
| _distributePerformanceFee(); |
There was a problem hiding this comment.
this can revert (if subvault.withdraw revert), preventing disabling fee when subvault have withdrawal paused
|
|
||
| __ReentrancyGuard_init(); | ||
| __Pausable_init(); | ||
| __MasterVaultRoles_init(); |
There was a problem hiding this comment.
local roles are not initialized and no one have local default admin role to grant roles
| * @title Layer 1 Gateway contract for bridging Custom ERC20s with YBB enabled | ||
| * @notice Escrows funds into MasterVaults for yield bearing bridging. | ||
| */ | ||
| contract L1YbbCustomGateway is L1CustomGateway { |
There was a problem hiding this comment.
Lack fee token (Orbit) variant, same for the L1YbbERC20Gateway
| 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 = asset.balanceOf(address(this)); |
There was a problem hiding this comment.
inconsistent use of _totalAssetsLessProfit and asset.balanceOf(address(this))
There was a problem hiding this comment.
can you explain more what you mean? i'm not sure i follow
There was a problem hiding this comment.
we're changing rebalancing to include profit assets too fwiw
There was a problem hiding this comment.
| if (idleAssets < assets) { | ||
| uint256 assetsToWithdraw = assets - idleAssets; | ||
| // slither-disable-next-line unused-return | ||
| subVault.withdraw(assetsToWithdraw, address(this), address(this)); |
There was a problem hiding this comment.
also lack subvault slippage check tho idk what is a good fix
There was a problem hiding this comment.
one of the assumption we have here is subvault should be erc4626 compliant, which require subvault.withdraw() to transfer exactly the requested assets a nd if a subVault misbehaves safeTransfer method on the next line would revert anyway
There was a problem hiding this comment.
The fact that it return EXACT mean rounding error are lost, while the user withdrawing will get full value. This should be fine bcz most vault have share value much lower than 1 wei, but using redeem can be safer (so the master vault get the rounding assets).
E.g. if 1 share = 100 wei, withdraw(1) burn 1 share and receive 1 wei, losing 99 wei
There was a problem hiding this comment.
i think we should leave this alone. as you say most vaults have low share values.
even if the value of the shares is high it's still an expensive thing to exploit to any meaningful degree because you'd have to initiate a separate withdrawal tx for every single subVault share you want to get rounded off.
there's not really a good way to profit from exploiting this unless you hold a non negligible share of the subvault or something like that
losing 1 subVault share's worth of assets on a rebalance, redeem, or fee distribution is acceptable imo
| /// @notice Set the target allocation of assets to keep in the subvault | ||
| /// @dev Target allocation must be between 0 and 1e18 (100%). | ||
| /// @param _targetAllocationWad The target allocation in wad (1e18 = 100%) | ||
| function setTargetAllocationWad(uint64 _targetAllocationWad) |
| onlyGateway | ||
| returns (uint256 shares) | ||
| { | ||
| shares = _convertToSharesRoundDown(assets); |
There was a problem hiding this comment.
imo we should implement an optional slippage check in the gateways
we could pass it by repurposing extraData which would leave the existing function signature unchanged. we could also introduce a new function which is probably a bit better
ybb gateway review
* handle fee on transfer tokens * document underlying token requirements * do not support FoT * test FoT
This reverts commit 692bdd7.
…erance YBB Gateways Optional Slippage Tolerance
YBB Happy E2E Tests
Revert "YBB Happy E2E Tests"
* deposit side of ybb happy case e2e tests * check mastervaultfactory set on custom gateway * ybb withdrawal e2e test * slippage path e2e tests * dry out ybb e2e tests * optimize absYbbNonReentrant * Revert "optimize absYbbNonReentrant" This reverts commit 692bdd7.
gzeoneth
left a comment
There was a problem hiding this comment.
Looking pretty good, some minor comments
| if (idleAssets < assets) { | ||
| uint256 assetsToWithdraw = assets - idleAssets; | ||
| // slither-disable-next-line unused-return | ||
| subVault.withdraw(assetsToWithdraw, address(this), address(this)); |
There was a problem hiding this comment.
The fact that it return EXACT mean rounding error are lost, while the user withdrawing will get full value. This should be fine bcz most vault have share value much lower than 1 wei, but using redeem can be safer (so the master vault get the rounding assets).
E.g. if 1 share = 100 wei, withdraw(1) burn 1 share and receive 1 wei, losing 99 wei
Co-authored-by: gzeon <hng@offchainlabs.com>
godzillaba
left a comment
There was a problem hiding this comment.
we need to confirm whether we need token name+symbol prefix+suffix
Uh oh!
There was an error while loading. Please reload this page.