Taiko DAO Contracts - Taiko

Throughout the files in scope, there are several instances where components are declared but never used. Instances of this issue include:

• In the StandardProposalCondition contract, the dao variable is declared but not used throughout the contract.

• In the Multisig contract the InvalidAddressListSource error is declared but never used.

• In the OptimisticTokenVotingPlugin contract the ProposalCreationForbidden error is declared but never used.

Additionally, it was identified that several imported contracts or interfaces are not used within the proposed scope.

- src/SignerList.sol

	import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";

- src/factory/TaikoDaoFactory.sol

	import {Addresslist} from "@aragon/osx/plugins/utils/Addresslist.sol";

	import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

- src/setup/EmergencyMultisigPluginSetup.sol

	import {DAO} from "@aragon/osx/core/dao/DAO.sol";

- src/setup/MultisigPluginSetup.sol

	import {DAO} from "@aragon/osx/core/dao/DAO.sol";

- src/setup/OptimisticTokenVotingPluginSetup.sol

	import {ITaikoL1} from "../adapted-dependencies/ITaikoL1.sol";

• Remove the unused dao variable from the StandardProposalCondition contract. Alternatively, if the variable is intended to be used, implement the necessary logic.

• Remove unused custom errors and ensure that any relevant error checks consistently reference active custom errors. This maintains a clean codebase, reduces confusion, and makes the contract’s logic clearer for future readers and maintainers.

  
  

• Remove unused imports in order to increase code maintainability and avoid unnecessary bloat.

SOLVED: The Taiko Labs team solved this finding in commit f80fb99 by following the mentioned recommendation.

The smart contracts in-scope include several functions that are declared as public but are not invoked internally within the smart contracts. These functions are intended to be called only from external sources.

- src/DelegationWall.sol

	    function register(bytes memory _contentUrl) public {

	    function getCandidateAddresses() public view returns (address[] memory) {

	    function candidateCount() public view returns (uint256) {

- src/EmergencyMultisig.sol

	    function supportsInterface(bytes4 _interfaceId) public {

	    function getProposal(uint256 _proposalId) public {

	    function hasApproved(uint256 _proposalId, address _account) public view returns (bool) {

	    function execute(uint256 _proposalId, bytes memory _metadataUri, IDAO.Action[] calldata _actions) public {

- src/EncryptionRegistry.sol

	    function appointAgent(address _newAgent) public {

	    function setOwnPublicKey(bytes32 _publicKey) public {

	    function setPublicKey(address _accountOwner, bytes32 _publicKey) public {

	    function getRegisteredAccounts() public view returns (address[] memory) {

	    function getAppointedAgent(address _account) public view returns (address) {

- src/Multisig.sol

	    function getProposal(uint256 _proposalId) public {

	    function hasApproved(uint256 _proposalId, address _account) public view returns (bool) {

	    function execute(uint256 _proposalId) public {

- src/OptimisticTokenVotingPlugin.sol

	    function hasVetoed(uint256 _proposalId, address _voter) public view returns (bool) {

	    function getProposal(uint256 _proposalId) public {

	    function veto(uint256 _proposalId) public virtual {

	    function execute(uint256 _proposalId) public virtual {

	    function updateOptimisticGovernanceSettings(OptimisticGovernanceSettings calldata _governanceSettings) public {

	    function parseProposalId(uint256 _proposalId) public {

- src/SignerList.sol

	    function isListedOrAppointedByListed(address _address) public view returns (bool listedOrAppointedByListed) {

	    function getListedEncryptionOwnerAtBlock(address _address, uint256 _blockNumber) public {

	    function resolveEncryptionAccountAtBlock(address _address, uint256 _blockNumber) public {

	    function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {

- src/factory/TaikoDaoFactory.sol

	    function deployOnce() public {

	    function getSettings() public view returns (DeploymentSettings memory) {

	    function getDeployment() public view returns (Deployment memory) {

To improve code clarity and optimize gas usage, it is recommended to change the visibility of these functions from public to external.

The external keyword is specifically designed for functions that are meant to be called from outside the contract, and it can result in more efficient code execution.

By making this change, you can enhance the readability and performance of the smart contracts.

SOLVED: The Taiko Labs team solved this finding in commit 939b67a by following the mentioned recommendation.

The EncryptionRegistry contract maintains an accountList array that stores all registered accounts, but lacks functionality to remove accounts that are no longer active or have been removed as appointed agents. Accounts are added to this accountList when they first appoint an agent or set a public key, but there is no corresponding mechanism to remove them when they become inactive or delisted.

As the accountList grows indefinitely, operations that iterate through this list (like checking for existing accounts in appointAgent() and _setPublicKey()) will consume increasingly more gas. This creates a potential DoS vector where the list becomes so large that operations become prohibitively expensive or hit block gas limits.

Implement a removal mechanism that allows accounts to be removed from the accountList when they are removed as appointed agents. This could be achieved through a cleanup during agent appointment or key setting operations.

SOLVED: The Taiko Labs team solved this finding in commit 2628cad by following the mentioned recommendation.

In the StandardProposalCondition contract, the dao and minDuration variables are missing the visibility modifier.

By default, variables are set to internal visibility. However, It is considered best practice to explicitly specify visibility to enhance clarity and prevent ambiguity. Clearly labeling the visibility of all variables and functions will help in maintaining clear and understandable code.

Explicitly define the visibility of all variables in the contracts to enhance readability and reduce the potential for errors.

SOLVED: The Taiko Labs team solved this finding in commit 62b1785 by following the mentioned recommendation.

Throughout the contracts in scope, there are several instances where input validation is missing. Instances if this issue include:

• The _governanceERC20Base and _governanceWrappedERC20Base parameters are not checked against the zero address in the OptimisticTokenVotingPluginSetup contract constructor.

• The _minDuration parameter is not validated to fall within a reasonable range in the StandardProposalCondition contract constructor. A long duration could potentially lock the contract for an excessive period.

• The _dao parameter in the initialize() function of the SignerList contract is not validated against the zero address.

• The _setPublicKey() function in the EncryptionRegistry contract does not validate the _publicKey parameter against a valid length.

• In the OptimisticTokenVotingPlugin contract, the votingToken and taikoBridge state variables are updated without checking whether the assigned address is the zero address (address(0)).

Implement input validation to ensure that the input parameters are valid and within the expected ranges.

PARTIALLY SOLVED: The Taiko Labs team partially solved this finding in commit a8fa1e5 by verifying the _governanceERC20Base and _governanceWrappedERC20Base parameters against the zero address.

In the OptimisticTokenVotingPlugin contract, there is an empty revert statement, as follows:

if (_taikoL1 == address(0)) revert();

In case the condition is met, the function call to the initialize() function will revert without a descriptive error message.

Consider creating a custom error for this specific condition, or reverting with a string (reason) for clarity.

SOLVED: The Taiko Labs team solved this finding in commit a8fa1e5 by following the mentioned recommendation.

The SignerList contract lacks validation against zero addresses when adding new signers through the initialize() and addSigners() functions. These functions rely on the internal _addAddresses() function inherited from the Addresslist contract, which does not perform zero address validation.

The addition of zero addresses could affect quorum calculations, by artificially inflating the number of available signers.

Implement zero address validation to prevent adding invalid addresses as signers.

ACKNOWLEDGED: The Taiko Labs team made a business decision to acknowledge this finding and not alter the contracts.

Several function calls throughout the contracts in scope ignore the return values of the functions they call. Ignoring return values can lead to unexpected behavior and may result in unanticipated outcomes.

Instances of unhandled return values include:

• In src/EmergencyMultisig.sol:

  proposal_.destinationPlugin.createProposal(

  
  

• In src/Multisig.sol

  proposal_.destinationPlugin.createProposal(

Ensure that the return values of external calls are handled appropriately throughout the contracts.

ACKNOWLEDGED: The Taiko Labs team made a business decision to acknowledge this finding and not alter the contracts.

Smart 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.

During the analysis of the proposed scope, it was identified that all smart contracts are using a floating pragma, as follows:

	pragma solidity ^0.8.17;

Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

ACKNOWLEDGED: The Taiko Labs team made a business decision to acknowledge this finding and not alter the contracts.

In the resolveEncryptionAccountAtBlock() function of the SignersList contract, the this keyword is used redundantly when calling the isListedAtBlock() function.

if (this.isListedAtBlock(_appointer, _blockNumber)) {
    // The appointed agent votes
    return (_appointer, _address);
}

While using the this keyword is not incorrect, it is redundant and may create gas overhead in this context.

Call the isListedAtBlock() function directly without using the this keyword.

SOLVED: The Taiko Labs team solved this finding in commit eb18176 by following the mentioned recommendation.

In the deployOnce() function of the TaikoFactory contract, state is modified. However, these changes are not reflected in any event emission. Additionally, in the execute() function of the OptimisticTokenVotingPlugin no events are emitted as well.

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

ACKNOWLEDGED: The Taiko Labs team made a business decision to acknowledge this finding and not alter the contracts.

In the SignerList contract there is a typo in an error name, where the word Registry is misspelled as Regitry.

error InvalidEncryptionRegitry(address givenAddress);

While this typo does not affect the functionality of the code, it can make the codebase harder to read and understand.

Correct the typo in the error name to improve the readability of the codebase.

SOLVED: The Taiko Labs team solved this finding in commit eb18176 by following the mentioned recommendation.