QA Report

[code423n4]

newblockchain QA report

Summary

• It was difficult to work on this project, due to lack of documentation/tests. Sometimes it was not clear what was the intention or intended usage was.
• Also some addresses were hardcoded, without pointing to a valid contract on the mainnet (unigov in lending-market, for example). It was hard to test.

Low

GovernorBravoDelegator cannot be used with GovernorBravoDelegate

• GovernorBravoDeleagtor.sol:39-58 Although there is no through documentation, based on the name, GovernorBravoDelegator should be used with GovernorBravoDelegate as implementation. However, due to lack of corresponding functions, some exposed functions will not work.
  • _acceptInitialAdmin does not exist in GovernorBravoDelegate
  • _initiate() does not exist in GovernorBravoDelegate but only _initiate(address governorAlpha). The number of arguments does not match.

Upon queue executed true in GovernorBravoDelegate

• link When a new proposal is queued, the executed is set to be true. This is checked in the state function and when true, it will return ProposalState.Executed. Which means any proposal which was queued can actually never be executed. However, due to lack of existing tests and time constraints, is not confirmed.

createPair lack of access control in BaseV1-core

• link The function createPair is used to deploy a contract for a token pairs. And any one can create such a pair, also decide to give stable flag true or false. It is unclear whether it was intended usage or not, nor it is tested and confirmed.

Non-Critical

Window should be enforced to be greater than 0 in BaseV1-core

• BaseV1-core.sol:218-235 In the sample function, the argument window should be checked to be greater than 0, otherwise, it might revert from division by zero.

COMP token address for WETH hardcoded in Comptroller

• Comptroller.sol:1465 Although, the comment says that the address is WETH address, the contract in the given address in mainnet is COMP contract.

Misleading comments in Proposal-Store.sol

• Proposal-Store.sol:6-7 The comment above ProposalStore does not describe the contract.

symbol is optional for ERC20 in BaseV1-core

• BaseV1-core.sol:109-113 As the ERC20 standard, symbol is not a must, but an optional function. Also, some token like MKR returns base32 for the symbol which will revert when used for the standard ERC20 interface. It is safer to use either wrapper or try catch.

[GalloDaSballo]

GovernorBravoDelegator cannot be used with GovernorBravoDelegate

This is incorrect per how the "delegation" works, via fallback meaning, existing function in the Delegator work normally

Upon queue executed true in GovernorBravoDelegate

Valid Low and potentially higher -> TODO: Bump

createPair lack of access control in BaseV1-core

Disagree as pair creation in an AMM is crucial and permissionless creation is an acceptable design decision

Window should be enforced to be greater than 0 in BaseV1-core

NC as it's self DOS

COMP token address for WETH hardcoded in Comptroller

Hardcoded is cheaper

Misleading comments in Proposal-Store.sol

NC

symbol is optional for ERC20 in BaseV1-core

Nice find, potentially higher sev - Low for now

Your effort shows and I think you did a good job, I'd recommend providing objective feedback over personal story-telling "I had issues with he codebase".

Perhaps: "The codebase could use more thorough commenting" would work better

2L 2NC