code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L515-L522

Vulnerability details

Impact

If the execute function here https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L283 is called at a valid time , i.e when currentTime = proposal_.parameters.endDate , it would revert because it calls _canExecute which calls _isProposalOpen which would return false.

Proof of Concept

Let's compare this logic to the Multisig implementation of the function,

function _isProposalOpen(Proposal storage proposal_) internal view returns (bool) {
        uint64 currentTimestamp64 = block.timestamp.toUint64();
        return
            !proposal_.executed &&
            proposal_.parameters.startDate <= currentTimestamp64 &&
            proposal_.parameters.endDate >= currentTimestamp64;

Here we can see if proposal_.parameters.startDate = currentTimestamp64 it won't return false , this is because this is a valid case (can be vice versa too , if it is considered that when they are equal it should revert then the latter is valid) , and execute function should work fine.

But inside the MajorityVotingBase https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L515-L522 , the check is not consistent

function _isProposalOpen(Proposal storage proposal_) internal view virtual returns (bool) {
        uint64 currentTime = block.timestamp.toUint64();

        return
            proposal_.parameters.startDate <= currentTime &&
            currentTime < proposal_.parameters.endDate &&
            !proposal_.executed;
    }

Here , when currentTime = proposal_.parameters.endDate , the function would return false , and the execute function would revert.

Tools Used

Manual analysis , VSCode, Hardhat

Recommended Mitigation Steps

Stick to one condition for the valid timestamps execute has to follow

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)

0xean commented 1 year ago

Looks to be an off by one type of error but I don't see how this qualifies as M given the impact statement from the warden.

c4-judge commented 1 year ago

0xean marked the issue as grade-b

novaknole20 commented 1 year ago

This poses no risk and plugins have to be seen independently. However, we agree that it is better if things are uniform and will accept this as a QA suggestion.

c4-sponsor commented 1 year ago

novaknole20 marked the issue as sponsor confirmed

code-423n4 / 2023-03-aragon-findings