Mainnet #29
Mainnet #29
Conversation
src/PufferVaultMainnet.sol
Outdated
| /** | ||
| * @param withdrawalAmount is the assets amount, not shares | ||
| */ | ||
| function _checkDailyWithdrawalLimits(uint256 withdrawalAmount) internal { |
There was a problem hiding this comment.
function name needs to reflect the functionality -- updates state variables. suggestion: checkandUpdate...
There was a problem hiding this comment.
Changed the logic for maxDeposit and maxRedeem so that we are compliant with the 4626 standard.
src/PufferVaultMainnet.sol
Outdated
| VaultStorage storage $ = _getPufferVaultStorage(); | ||
|
|
||
| // If daily withdrawal limit is 0, then there is no limit | ||
| if ($.dailyWithdrawalLimit == 0) { |
There was a problem hiding this comment.
dailyWithdrawalLimit should be maxUInt instead for no limit case. (dailyWithdrawalLimit == maxUint --> return )
(maybe set dailyWithdrawalLimit in initializer to 0 for explicit declaration)
There was a problem hiding this comment.
I think this is also fixed by changing the logic to use default 4626 limits, I'll resolve this after reviewing the new flow.
src/PufferVaultMainnet.sol
Outdated
| * @notice Allows the `msg.sender` to burn his shares | ||
| * @param shares The amount of shares to burn | ||
| */ | ||
| function burn(uint256 shares) public { |
There was a problem hiding this comment.
Add restricted modifier to make it Pausable (consistent with the rest of the functions)
| @@ -24,6 +24,7 @@ import { ILidoWithdrawalQueue } from "src/interface/Lido/ILidoWithdrawalQueue.so | |||
| import { IEigenLayer } from "src/interface/EigenLayer/IEigenLayer.sol"; | |||
| import { IStrategy } from "src/interface/EigenLayer/IStrategy.sol"; | |||
| import { Timelock } from "src/Timelock.sol"; | |||
| import { IWETH } from "src/interface/Other/IWETH.sol"; | |||
There was a problem hiding this comment.
Todo: integration testing with Weth, flows such as deposit, withdraw, redeem, etc
as well as some edge cases such as:
- withdraw too much
- different balances (weth, eth) v.s. diff withdrawalAmounts
- withdrawalLimits changes
src/PufferVaultMainnet.sol
Outdated
| */ | ||
| function maxWithdraw(address owner) public view virtual override returns (uint256 maxAssets) { | ||
| uint256 remainingAssets = getRemainingAssetsDailyWithdrawalLimit(); | ||
| uint256 maxUserAssets = _convertToAssets(balanceOf(owner), Math.Rounding.Floor); |
There was a problem hiding this comment.
Use previewRedeem() for consistency with the rest of the code.
src/PufferVaultMainnet.sol
Outdated
| erc4626Storage._asset = _WETH; | ||
|
|
||
| VaultStorage storage $ = _getPufferVaultStorage(); | ||
| $.lastWithdrawalDay = uint64(block.timestamp / 1 days); |
There was a problem hiding this comment.
The daily limit needs to be set to 0, as in withdrawal is disabled.
(We need to explicitly set that to 0 here for readability and auditors.)
src/PufferVaultMainnet.sol
Outdated
|
|
||
| function getRemainingAssetsDailyWithdrawalLimit() public view virtual returns (uint96) { | ||
| VaultStorage storage $ = _getPufferVaultStorage(); | ||
| return $.dailyAssetsWithdrawalLimit - $.assetsWithdrawnToday; |
There was a problem hiding this comment.
In the scenario that the limit is not 0 and is set to 0, then this would be negative and revert, resulting in other functions failing.
add a check
$.dailyAssetsWithdrawalLimit<$.assetsWithdrawnTodayreturn 0- else ...
| @@ -0,0 +1,97 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
There was a problem hiding this comment.
@bxmmm1 is there a test that shows this upgrade works as expected? E.g. we can call the upgrade on the vault and try calling some of the functions in a test
There was a problem hiding this comment.
|
LGTM, my main suggestions are to drop the swapping logic and keep the stETH redemption logic from V1 contracts. |
No description provided.