Hey, just some comments on things I understand as best practices / potential future proofing when extending this codebase. Didn't really have the time to go deep this round.
Controller.solsetInflationManager, addStakerVault and removePool should probably emit events indicating the changes made, as they are sensitive actions governance can make.
CvxCrvRewardsLocker.solsetTreasury does not check for zero address - uninitialized parameters by human error would end up with burned tokens if followed by any withdraw to treasury.
GasBank.solwithdrawUnused and withdrawFrom allow any action to withdraw from any account. Currently not a problem because only the TopUpAction exists, but there should probably be some kind of mapping between actions and the accounts they can affect / manage.
RoleManager.sol
External functions are protected by the onlyGovernance modifier and call internal _grantRole / _revokeRole. removeGaugeZap should probably follow the same convention (currently not modifier protected, but instead calls public revokeRole, which is)
TopUpAction.sol
TopUpActionLibrary's lockFunds doesn't use the addressProvider pattern existing in the codebase. Nothing ensures that stakerVaultAddress corresponds to token. Not currently dangerous because only TopUpAction::_lockFunds calls this function, appropiately getting the vault address from the addressProvider, but should probably future proof by doing it inside as the other functions in the library.
BkdTriHopCvx.soldecimalMultiplier assumes tokens have <= 18 decimals. Will always be 0 for tokens with more than 18 (which would be still ERC20 compliant).
StrategySwapper.sol_decimalMultiplier assumes tokens have <= 18 decimals. Will always be 0 for tokens with more than 18 (which would be still ERC20 compliant).
InflationManager.solsetMinter does not emit an event whilst changing the allowed minter address (a sensitive, governance protected action).
Hey, just some comments on things I understand as best practices / potential future proofing when extending this codebase. Didn't really have the time to go deep this round.
CvxCrvRewardsLocker.sol setTreasury does not check for zero address - uninitialized parameters by human error would end up with burned tokens if followed by any withdraw to treasury.
GasBank.sol withdrawUnused and withdrawFrom allow any action to withdraw from any account. Currently not a problem because only the TopUpAction exists, but there should probably be some kind of mapping between actions and the accounts they can affect / manage.
RoleManager.sol External functions are protected by the onlyGovernance modifier and call internal
_grantRole
/_revokeRole
. removeGaugeZap should probably follow the same convention (currently not modifier protected, but instead calls publicrevokeRole
, which is)TopUpActionLibrary's lockFunds doesn't use the addressProvider pattern existing in the codebase. Nothing ensures that
stakerVaultAddress
corresponds totoken
. Not currently dangerous because onlyTopUpAction::_lockFunds
calls this function, appropiately getting the vault address from the addressProvider, but should probably future proof by doing it inside as the other functions in the library.BkdTriHopCvx.sol decimalMultiplier assumes tokens have <= 18 decimals. Will always be 0 for tokens with more than 18 (which would be still ERC20 compliant).
StrategySwapper.sol _decimalMultiplier assumes tokens have <= 18 decimals. Will always be 0 for tokens with more than 18 (which would be still ERC20 compliant).
InflationManager.sol setMinter does not emit an event whilst changing the allowed minter address (a sensitive, governance protected action).
Cheers!