code-423n4 / 2022-08-nounsdao-findings

2 stars 0 forks source link

The variable value ```uint256 proposalCount``` is reset when migrating from V1 to V2 #321

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOInterfaces.sol#L347

Vulnerability details

Context:

NounsDAOInterfaces.sol

contract NounsDAOStorageV1 {
/// The total number of proposals for V1 
uint256 public proposalCount;
}
contract NounsDAOStorageV1Adjusted {
/// The total number of proposals for V2
uint256 public proposalCount;
}

Description:

The uint256 proposalCount variable value is defined in V1 and V2 Logic contracts as above, and each proposal is incremented by 1 with proposalCount++;. However, when switching to V2 Logc contract, the proposalId value is not preserved and will start from 1 again. Proposals that were created on V1 and will remain active post-upgrade, should behave the same

GovernorBravoDelegateG2.sol#L367-L373 This feature is clearly stated in Compound's GovernerBravo contract, we see that the V2 Logic contract does not add the _initiate function, this function solves exactly this problem

The project team deliberately removed this detail from the code in the first version, but did not include it in LogicV2, it is mentioned in the explanations as follows;NounsDAOLogicV1.sol#L51-L54

uint256 proposalCount In order to keep the current variable value of NounsDAOStorageV1, it is necessary to modify the architecture of the NounsDAOInterfaces.sol contract, NounsDAOStorageV1 should be added to the inherit hierarchy below

alt tag

Recommendation: add this function


 function _initiate(address governorAlpha) external {
        require(msg.sender == admin, "NounsDAO: initiate: admin only");
        require(initialProposalId == 0, "NounsDAO: initiate: can only initiate once");
        proposalCount = GovernorAlpha(governorAlpha).proposalCount();
        initialProposalId = proposalCount;
        timelock.acceptAdmin();
    }
davidbrai commented 2 years ago

I don't see why the proposalCount variable would change to zero during the upgrade.