Platform: Code4rena
Start Date: 03/03/2023
Pot Size: $90,500 USDC
Total HM: 4
Participants: 42
Period: 7 days
Judge: 0xean
Total Solo HM: 2
Id: 219
League: ETH
Rank: 22/42
Findings: 1
Award: $72.43
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: rbserver
Also found by: 0x6980, 0xAgro, 0xSmartContract, 0xmichalis, 0xnev, BRONZEDISC, DevABDee, IceBear, RaymondFam, Rolezn, SaeedAlipoor01988, Sathish9098, arialblack14, brgltd, chrisdior4, codeislight, descharre, imare, lukris02, luxartvinsec, matrix_0wl, tnevler, yongskiws
72.4344 USDC - $72.43
Multiple functions in the project emit an event as the last statement. Wherever possible, consider emitting events before external calls. In case of reentrancy, funds are not at risk (for external call + event ordering), however emitting events after external calls can damage frontends and monitoring tools in case of reentrancy attacks.
The project is using v0.8.17. If possible, consider updating to the latest stable version v0.8.19.
Consider updating from OpenZeppelin 4.8.1 to 4.8.2 to ensure the contracts contain the latest security fixes.
Lack of two-step procedure for critical operations leaves them error-prone.
Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critical functionalities to the wrong address.
It's possible to define the __gap into a single contract and reuse across the other contracts. Please, refer to this reference implementation
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L339
weth will be more composable and easier to reason about in general. This is more of an arquitectural decision, but it can have security ramifications. Please, refer to this video where MakerDAO recommends using weth instead of eth.
For DAORegistry.register(), the argument dao is shadowed by DaoAuthorizableUpgradeable.dao(), Consider renaming the dao argument in DAORegistry.register().
Add a check ensuring that the new value if different than the current value to avoid emitting unnecessary events.
Adding events will facilitate offchain monitoring.
DaoAuthorizableUpgradeable.sol to DAOAuthorizableUpgradeable.solIt will be more consistent with other filenames, e.g. DAO.sol.
Some functions return named variables, others return explicit values.
Following function returns an explicit value.
Following function return returns a named variable.
Consider adopting the same approach throughout the codebase to improve the explicitness and readability of the code.
The solidity documentation recommends the following order for functions:
constructor receive function (if exists) fallback function (if exists) external public internal private
Consider moving receive and fallback above external functions.
The solidity documentation recommends a maximum of 120 characters.
Consider breaking down comments into multiple lines to avoid having large line lengths.
https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L134
OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.
Consider waiting until the contract is finalized. Otherwise, make sure that development team is aware of the risks of using a draft OpenZeppelin contract and accept the risk-benefit trade-off.
It's possible to name the imports for OpenZeppelin dependencies.
#0 - c4-judge
2023-03-12T16:27:13Z
0xean marked the issue as grade-b
#1 - novaknole20
2023-03-22T10:58:22Z
5: Nope the gap can be different for each contract.
6: Nope
#2 - c4-sponsor
2023-03-22T10:58:27Z
novaknole20 marked the issue as sponsor acknowledged