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/addresslist/AddresslistVoting.sol#L120-L123 https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/utils/Ratio.sol#L13-L29

Vulnerability details

Impact

Truncation to zero could easily happen in Solidity if the numerator happens to be smaller than the denominator even by just a little bit, e.g. the difference between 1e6 (denominator) and 1e6 - 1 (numerator) in the instance of this report.

Proof of Concept

In createProposal() of AddresslistVoting.sol, the function logic makes a pure function call to the imported _applyRatioCeiled().

File: AddresslistVoting.sol#L120-L123

        proposal_.parameters.minVotingPower = _applyRatioCeiled(
            totalVotingPower(snapshotBlock),
            minParticipation()
        );

File: Ratio.sol#L13-L29

/// @notice Applies a ratio to a value and ceils the remainder.
/// @param _value The value to which the ratio is applied.
/// @param _ratio The ratio that must be in the interval `[0, 10**6]`.
/// @return result The resulting value.
function _applyRatioCeiled(uint256 _value, uint256 _ratio) pure returns (uint256 result) {
    if (_ratio > RATIO_BASE) {
        revert RatioOutOfBounds({limit: RATIO_BASE, actual: _ratio});
    }

    _value = _value * _ratio;
    uint256 remainder = _value % RATIO_BASE;
    result = _value / RATIO_BASE;

    // Check if ceiling is needed
    if (remainder != 0) {
        ++result;
    }

As can be seen from the code blocks above, it is possible that minParticipation() is associated with a small enough value to yield a numerator product totalVotingPower(snapshotBlock) * minParticipation() that is smaller than the numerator RATIO_BASE. Consequently, an unexpected zero value is returned and assigned to proposal_.parameters.minVotingPower that may likely mess up with the voting process of the created proposal since literally any member with a minimum voting power of zero or above is eligible to vote.

(Note: Neither totalVotingPower(snapshotBlock) nor minParticipation() have been scaled in the codebase of the protocol).

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider scaling the numerator value in _applyRatioCeiled() of Ratio.sol or one of the input arguments from AddresslistVoting.sol, i.e. totalVotingPower(snapshotBlock) or minParticipation().

0xean commented 1 year ago

dupe of https://github.com/code-423n4/2023-03-aragon-findings/issues/40 - also QA I believe.

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)