0xfoobar
commented
2 years ago Sponsor disputed
- floating pragmas are useful for testing version upgrades against the same codebase
- transferOwnership to a zero address is useful for burning the admin keys
- a two-step process is certainly possible but we decided against it here
0xleastwood
Floating pragma
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103
Lines of code
Pretty much all the contracts, some of which are below https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV1.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L2 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L1
Missing non-zero address check
It is a good practice to include non-zero address check especially when updating important addresses. I suggest to include a non-zero address check for transferOwnership()
Lines of code
https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L39
It would be better to make transferOwnership() as a two-step process
transferOwnership() is called by the current owner to change the owner address. It can be a better approach to follow a 2-step process when updating such priviliged addresses First transaction proposes the pending owner address, second transaction which can only be called by the proposed address accepts the role. A similar 2-step approach is implemented in AlchemistV2.sol for admin update using setPendingAdmin() and acceptAdmin().
Lines of code
https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L39