Integrate PufferVault #158
Conversation
bxmmm1
commented
Jan 24, 2024
•
bxmmm1
commented
- Add PufferVault
- Remove PufferPool
- Remove WithdrawalPool
- Daily limit for withdrawals
- Validator Tickets
Integrate PufferVault
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
* erc20 permit vt * add vt functionality and forge fmt * make mint function pausable * cleanup comments --------- Co-authored-by: shcyster <[email protected]>
src/PufferOracle.sol
Outdated
| } | ||
|
|
||
| function _setMintPrice(uint56 newPrice) internal { | ||
| uint256 oldPrice = _validatorTicketPrice; |
There was a problem hiding this comment.
remove the extra variable, you can emit the event first and then update the global variable
script/UpgradeProtocol.s.sol
Outdated
| @@ -16,14 +17,14 @@ import { WithdrawalPool } from "puffer/WithdrawalPool.sol"; | |||
| */ | |||
There was a problem hiding this comment.
Remove the mention of PufferPool within the comment on line 14:
@notice Calls the depositETH function on PufferPool
| * Slot 1 | ||
| */ | ||
| uint256 public lockedETH; | ||
| /** |
There was a problem hiding this comment.
newline whitespace please :)
src/PufferProtocol.sol
Outdated
|
|
||
| // We've received bond in pufETH, so the second param is 0 | ||
| _transferFunds($, 0); | ||
| function withdrawValidatorTicket(uint256 amount, address recipient) external { |
There was a problem hiding this comment.
We do not need withdrawValidatorTicket function. They will receive their validator tickets
- When they are skipped via skipProvisioning()
- When they call retrieveBond()
src/PufferProtocol.sol
Outdated
| Permit calldata pufETHPermit, | ||
| Permit calldata vtPermit | ||
| ) external payable restricted { | ||
| //@todo what is the min amount? |
There was a problem hiding this comment.
The min is 28 days, as you have it. But please make it a variable that can be controlled by the DAO
src/PufferProtocol.sol
Outdated
| @@ -359,14 +275,13 @@ contract PufferProtocol is IPufferProtocol, AccessManagedUpgradeable, UUPSUpgrad | |||
| } | |||
|
|
|||
| // slither-disable-next-line unchecked-transfer | |||
| POOL.transfer(validator.node, validator.bond); | |||
| PUFFER_VAULT.transfer(validator.node, validator.bond); | |||
There was a problem hiding this comment.
Can we please change the name of this function to cancelRegistration?
stopRegistration sounds a bit like they are exiting their validator, and it can be a bit confusing
src/PufferProtocol.sol
Outdated
| @@ -796,50 +618,13 @@ contract PufferProtocol is IPufferProtocol, AccessManagedUpgradeable, UUPSUpgrad | |||
| emit ValidatorLimitPerModuleChanged(oldLimit, limit); | |||
There was a problem hiding this comment.
You can emit the event first so you don't need to declare a separate variable to hold the oldLimit
function _setValidatorLimitPerModule(bytes32 moduleName, uint128 limit) internal {
ProtocolStorage storage $ = _getPufferProtocolStorage();
emit ValidatorLimitPerModuleChanged($.moduleLimits[moduleName].allowedLimit, limit);
$.moduleLimits[moduleName].allowedLimit = limit;
}
src/PufferProtocol.sol
Outdated
| ProtocolStorage storage $ = _getPufferProtocolStorage(); | ||
| bytes32[] memory oldModuleWeights = $.moduleWeights; | ||
| $.moduleWeights = newModuleWeights; | ||
| emit ModuleWeightsChanged(oldModuleWeights, newModuleWeights); |
There was a problem hiding this comment.
Similarly here, you can emit the event first so you don't have to declare an extra variable
| // DAO selectors | ||
| bytes4[] memory daoSelectors = new bytes4[](1); | ||
| daoSelectors[0] = PufferOracle.setMintPrice.selector; | ||
|
|
There was a problem hiding this comment.
We need to set the access for calling setGuardiansFeeRate()
| uint256 numberOfActivePufferValidators, | ||
| uint256 totalNumberOfValidators, | ||
| bytes[] calldata guardianSignatures | ||
| ) external { |
There was a problem hiding this comment.
I know we need valid Guardian signatures for this function, which should be hard to forge, but should we also make this function restricted? We really don't need anybody else calling it, do we?
| eigenPodManager = vm.envOr("EIGENPOD_MANAGER", address(new EigenPodManagerMock())); | ||
| delayedWithdrawalRouter = vm.envOr("DELAYED_WITHDRAWAL_ROUTER", address(0)); | ||
| delegationManager = vm.envOr("DELEGATION_MANAGER", address(new DelegationManagerMock())); | ||
| } | ||
|
|
||
| validatorTicketProxy = new ERC1967Proxy(address(new NoImplementation()), ""); |
There was a problem hiding this comment.
In theory, people could "steal" this proxy by backrunning our tx to create this contract to do their own authorizeUpgrade() call. We could consider changing this function within NoImplementation contract to require the address that calls that function to be the same as the deployer (since our deployment scripts basically just have the deployer deploy then upgrade).
| } | ||
|
|
||
| emit FullWithdrawalsRootPosted(blockNumber, root); | ||
| } | ||
|
|
||
| /** | ||
| * @inheritdoc IPufferProtocol | ||
| * @dev Restricted to the DAO |
There was a problem hiding this comment.
Make sure there is a comment like this, specifying who the function is restricted to, for all such functions with restricted modifier. I see there are some which do not have it e.g. createPufferModule and setModuleWeights
* vt accounting * update retrieveBond * add todo comments * VT Alternative (#167) * alternative code * penalize cancel and skip provisioning * Descriptions, todos * add stop validator virtual VTs * add virtual VT balance * rename variables, cleanup * rename variable, add assertion * PR changes * more comments * Guardian Sig to Include VT Burn Offset (#169) * add double deposit test * test double deposit alice for bob * update guardian signature to include vt burn offset * update function param comments --------- Co-authored-by: Cheyenne Atapour <[email protected]>
* fix proof of rewards interface * fix signatures * update selectors --------- Co-authored-by: Benjamin <[email protected]>
* fix slither error * update setup access * refactor to use struct * update redemption logic * add comments, refactor burning * ci update * ci fix
