The project consists of several smart contracts for the implementation of a DeFi DAO (Decentralized Autonomous Organization) using the Beacon Proxy pattern (upgradeable contracts).

The DAO is governed entirely by its individual members who collectively submit proposals (empty or ERC-20/ETH transfer). Each holder of a voting token (MiniMeToken) has the right to create proposals and vote for them. The number of votes is determined by the balance of the voting token holder at the time the proposal is created. Everyone can execute an accepted proposal.

In addition, the project has an ERC-721 token contract and an access control contract for delegating the right to create these tokens. ERC-721 token holders are capable to veto proposals.

The project has libraries for safe uint conversion, errors output and the description of a voting token properties.

All vulnerabilities discovered during the audit are classified based on their potential severity and have the following classification:

During the audit process, 2 CRITICAL, 2 HIGH, 1 MEDIUM and 6 LOW severity findings were spotted and acknowledged.

• Contract: VotingDAOV2.sol
• Lines: 170-174, 243

if (proposal.payment.amount > 0 && proposal.isAccepted()) {
    _withdraw(proposal.payment);
}

proposal.isExecuted = true;

destination.call{value: amount}("");

An attacker could specify a non-contract address as the destination address when creating a proposal for withdrawing ETH. Howewer, an attacker is able to deploy contract with the exact address using CREATE/CREATE2 pattern. If such a proposal is accepted and the attacker's contract has a fallback payable function which calls execute function again, the attacker will be able to steal all the ETH from the contract.

It's recommended to place the proposal.isExecuted setting before the if statement. Also, if call is not needed in this case, it can be changed to transfer.

• Contract: VotingDAOV2.sol
• Lines: 170-174, 259

if (proposal.payment.amount > 0 && proposal.isAccepted()) {
    _withdraw(proposal.payment);
}

proposal.isExecuted = true;

token.transfer(destination, amount);

An attacker is able to provide a poisoned token address when creating a proposal for withdrawing ERC-20. If the proposal is accepted, then the behaviour of this token during the transfer operation may be unexpected. For example. the function can always revert, which can lead to a situation that this proposal will be impossible to delete from the current contract implementation, the function can call the execute function again (reentrancy) that can lead to the theft all tokens from the contract, the function can transfer nothing which will lead to the fact that users will be deceived etc.

It is recommended to add a list of whitelisted ERC-20 tokens that are allowed to be added to the proposal. To protect against reentrancy for withdrawing ERC-20, as in the previous case, the proposal.isExecuted must be set before the if statement.

Files:

• ProposalQueue.sol (line 51)
• ProposalLibrary.sol (line 54-56)

function isActive(Proposal storage proposal) internal view returns (bool) {
    return proposal.createdBlockNumber > 0 && !isRejected(proposal) && !proposal.isExecuted;
}

Created proposals that were only rejected or executed are removed from the proposal queue at the time a new proposal is created. Expired proposals cannot be voted on or executed. This has the effect that if the proposal is expired and not rejected or accepted, it will never be removed from the proposal queue. With 10 such expired proposals in the proposal queue (not rejected and not executed) none of the functions of the VotingDAOV2 contract can be called (vote, veto or execute cannot be called if the proposal is expired and it is not possible to create new proposals).

It is recommended to add an additional condition !isExpired(proposal) to the return statement of the isActive function of the ProposalLibrary to check if the proposal has expired. It allows to remove expired proposals from the proposal queue when a new proposal is created.

Files:

• VotingDAOV2.sol (line 104)
• ProposalQueue.sol (line 51)
• ProposalLibrary.sol (line 54-56)

Proposals that have been vetoed cannot be voted on or executed. Howewer, vetoed proposals can only be removed from the proposal queue when a new proposal is created only if the proposal was vetoed after it had been rejected. In other cases. vetoed proposal will be considered as active in isActive function of the ProposalLibrary. With 10 such vetoed proposals in the proposal queue (not rejected by users before vetoed) none of the functions of the VotingDAOV2 contract will be able to be called (vote, veto or execute is not allowed to be called if the proposal is vetoed and it is not possible to create new proposals).

It is recommended to add additional condition !proposal.vetoed to the return statement of the isActive function of the ProposalLibrary to check if the proposal has been vetoed. It allows to remove vetoed proposals from the proposal queue when a new proposal is created.

If the private key of any VetoNFT holder would be exposed or at least one NFT would fall into the wrong hands, an attacker can veto any proposal at any time (if it is not expired and executed yet). Also, even if a proposal has been accepted but not executed yet, any NFT holder can veto it.

Consider adding multisig or voting pattern for veto functionality. If this option is not suitable consider transferring the NFT from the holder when vetoing, it will limit the number veto times. Also there is a possibility to add veto role to the access control contract.

Consider veto forbiddance if the proposal.isQuorumReached() returns true.

There is receive() external payable function in the VotingDAOV2.sol. But there is no functionality for the privileged person to return ETH/tokens, except through creating a proposal in case of an emergency.

It is recommended to add functionality for the ability to withdraw the remaining ETH/tokens from the balance of the VBeaconProxy contract for privileged persons.

There are missed events for veto, vote functions in VotingDAOV2.sol.

It is recommended to emit events for these functions.

• Variable uint32 updatedBlockNumber in Proposal.sol, line 20 - only updating when vote function is called from VotingDAOV2, never used.
• Function countYeas in ProposalQueue.sol, line 18 - never used.
• Require proposal.isQuorumReached() in VestingDAOV2.sol, line 167 - unnecessary check, it follows from the next line require: proposal.isAccepted().
• Condition proposal.isAccepted() in VestingDAOV2.sol, line 170 - always true, it follows from the require proposal.isAccepted(),
• Requires destination != address(0) and address(destination) != address(0) in VestingDAOV2.sol, lines 235, 252 - unnecessary checks, it follows from the require payment.destination != address(0) in _createProposal function.

It is recommended to delete unnecessary code.

• Contract: ProposalQueue.sol
• Lines: 30, 39

++i costs less gas compared to i++ or i += 1 for unsigned integers. In i++, the compiler has to create a temporary variable to store the initial value. This is not the case with ++i in which the value is directly incremented and returned, thus, making it a cheaper alternative.

It is recommended to change the post-increments (i++) to the pre-increments (++i).

• Contract: VotingDAOV2.sol
• Lines: 72, 88

It is suggested that the function names withdrawETH and withdrawToken reflect of their essence - creating a proposal for withdrawing ETH and tokens respectively. Howewer, if the parameter amount is set to 0, these functions operate just like createProposal function, which creates an empty proposal. It is a little bit confusing and there is also redundancy in the options for creating an empty proposal.

It is recommended to add require amount > 0 to the withdrawETH and withdrawToken functions.

• Contract: VotingDAOV2.sol
• Line: 124

In case of re-voting for the same decision, the transaction will not be reverted, and the user will spend money only on paying for gas. If this action is disabled, then the probability that user will not complete such a transaction increases (due to the MetaMask notification, HardHat local revert etc).

It's recommended to add revert for the transaction if the user wants to vote for the same decision in some proposal.