Revolution Protocol - Ward's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/base/VotesUpgradeable.sol#L206-L213 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/NontransferableERC20Votes.sol#L29 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/base/erc20/ERC20VotesUpgradeable.sol#L24

Vulnerability details

Impact

As stated in the comment on line 12 of NontransferableERC20Votes.sol, delegation of vote power can be done through the delegate function or by providing a signature to be used with delegateBySig. However, these functions do not prevent users from delegating to address(0), leading users to permanently lose their voting power, intentionally or accidentally.

Proof of Concept

A test demonstrating that delegation to address(0) is possible and indeed results in the loss of voting power. You can paste this into NontransferableERC20.t.sol. Also see: Issue in Nouns Builder repo for the same bug https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/203

function testVotingAndDelegationToAddress0() public {
        address delegate = address(0);
        uint256 mintAmount = 1000 * 1e18;

        // Mint tokens to the owner
        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(address(this), mintAmount);
        vm.stopPrank();

        // Delegate voting power
        erc20Token.delegate(delegate);

        // @bug Voting power is lost.
        assertEq(erc20Token.getVotes(delegate), 0);

        // Ensure that no tokens were transferred in the process of delegation
        assertEq(erc20Token.balanceOf(delegate), 0, "Delegation should not transfer tokens");
}

Tools Used

Manual Review

Recommended Mitigation Steps

Don't allow delegation to address(0) by adding checks to both delegate and delegateBySig functions as the following:

require(delegatee != address(0), "Votes: delegate to the zero address");

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L171-L200

Vulnerability details

Impact

Without ensuring the sequencer's uptime or validating the transaction order, there's a risk of transaction reordering and delay of the settlement of auctions. This could allow someone to manipulate the order in which bids are placed or settled. For instance an attacker could potentially front-run bids, impacting the fairness and outcome of the auction. Also see: https://github.com/sherlock-audit/2023-06-Index-judging/issues/40, lack of sequencer uptime may negatively impact English Auctions as I have described, as well as Dutch auctions.

Tools Used

Manual Review

Recommended Mitigation Steps

Determine the maximum tolerable delay for the sequencer and invalidate the auction if the sequencer was down for maximum tolerable delay or more during the auction period.

Assessed type

Other