c4-bot-3 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L274-L276 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L292-L298

Vulnerability details

Impact

The inaccuracy of (block.number) will affect the computation of the holding duration for the votes. That will affect _getPastVotes(), getPastVotes() functions and subsequently to the piece voting flow..

Proof of Concept

On Optimism, the block.number is not a reliable source of timing information and the time between each block is also different from Ethereum. This is because each transaction on L2 is placed in a separate block and blocks are not produce at a constant rate. This will cause the holding duration computation using _getPastVotes() to fluctuate. (see Optimism docs https://community.optimism.io/docs/developers/build/differences/#block-numbers-and-timestamps)

    /**
     * @notice Returns the voting power of a voter at the current block.
     * @param account The address of the voter.
     * @return The voting power of the voter.
     */
    function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) {
        return _getPastVotes(account, blockNumber);
    }

        function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
        return
            _calculateVoteWeight(
                erc20VotingToken.getPastVotes(account, blockNumber),
                erc721VotingToken.getPastVotes(account, blockNumber)
            );
    }

Recommendation migration Steps

Use block.timestamp rather than block.number for more accurate measurement of time.

Assessed type

Timing

c4-pre-sort commented 10 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #277

c4-judge commented 9 months ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 9 months ago

MarioPoneder marked the issue as grade-c

radeveth commented 9 months ago

Hey, @MarioPoneder!

I strongly believe that this issue should be considered a valid Medium Severity issue. In my report, I clearly explain how the inaccuracy of block.number affects the computation of the holding duration for votes. This impacts the _getPastVotes() and getPastVotes() functions, and subsequently, the piece voting flow.

I respectfully request a reassessment of the issue, considering its significant impact. I feel its severity may have been underestimated.

Cheers!

P.S. Here are similar vulnerabilities that were considered valid Medium Severity Issues in previous Code4rena Contests:

MarioPoneder commented 9 months ago