Bad Implementation of Vote Casting in OZVotingAdaptor

Summary

Users employing OZVotingAdaptor to participate in the voting process for proposals will encounter failures.

Vulnerability Detail

In the current implementation of OZVotingAdaptor, any user has the ability to create a proposal, and other users within the protocol can cast their votes using the vote(bytes calldata params) function. However, this function is currently not implemented correctly, leading to a failure for all users except the first one who utilizes this version of OZVotingAdaptor.

This issue arises because when a user invokes vote(), a subsequent call is made to the governor: IGovernor(governor).castVote(proposalId, support). During this call to cast the vote, we invoke the function _castVote(proposalId, voter, support, ""), where the voter is determined by msg.sender — the entity initiating the transaction.

Vote function in OZVotingAdaptor

    function vote(bytes calldata params) external {
        (uint256 proposalId, uint8 support) = abi.decode(params, (uint256, uint8));
        IGovernor(governor).castVote(proposalId, support);
    }

castVote function in governor.

    function castVote(uint256 proposalId, uint8 support) public virtual override returns (uint256) {
        address voter = _msgSender();
        return _castVote(proposalId, voter, support, "");
    }

In the OZVotingAdaptor implementation, the value of _msgSender() remains consistent, as we make the call from the adaptor and do not directly cast our votes. This leads to unexpected behavior, as the voter's identity is not accurately determined when using OZVotingAdaptor.

Here is a POC showcasing the exact scenario where users will encounter failure using the adaptor contract and when casting votes directly.

    function testVoteWillFail() public  {

        vm.prank(USER); // connect as normal USER 

        uint256 proposalId = createProposal(); //creating a new proposal for the protocol
         vm.roll(block.number + 1); // rolling to the next block to enable voting

         ozVotingAdaptor.vote(ozVotingAdaptor.encodeVoteCallData(proposalId, uint8(VoteType.For))); //voting with For to our proposal

        vm.prank(USER2); // connect as USER2 
        vm.roll(block.number + 1);  // rolling to the next block 

       //@audit-info the call will fail and USER2 wont be able to vote
        ozVotingAdaptor.vote(ozVotingAdaptor.encodeVoteCallData(proposalId, uint8(VoteType.For)));

        vm.prank(USER3); // connect as  USER3 
        vm.roll(block.number + 1);  // rolling to the next block 

        //@audit-info the call will fail and USER3 wont be able to vote
        ozVotingAdaptor.vote(ozVotingAdaptor.encodeVoteCallData(proposalId, uint8(VoteType.For)));

        vm.prank(USER2); // connect as  USER2 
        vm.roll(block.number + 1);  // rolling to the next block 
         //@audit-info this call wont fail and USER2 will be able to vote since he casted vote directly using the governor and not ozVotingAdaptor
        governor.castVote(proposalId, uint8(VoteType.For));

        vm.prank(USER3); // connect as  USER3 
        vm.roll(block.number + 1);  // rolling to the next block 
         //@audit-info this call wont fail and USER3 will be able to vote since he casted vote directly using the governor and not ozVotingAdaptor
        governor.castVote(proposalId, uint8(VoteType.For));

    }

Impact

This vulnerability severely hinders the voting process, allowing only the initial participant to successfully cast their vote. Subsequent users attempting to participate in the voting will inevitably encounter failures due to the flawed implementation in the OZVotingAdaptor.

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/adaptors/OZVotingAdaptor.sol#L63-L66

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/adaptors/OZVotingAdaptor.sol#L70-L73

Tool used

Manual Review

Recommendation

Since the current implementation of the vote() function in OZVotingAdaptor lacks additional logic and essentially mirrors the original castVote() function, serving merely as a call, I strongly recommend for its removal.

Given that we correctly declare and initialize the governor in the constructor, the vote() functionality will be readily available to all users without the necessity of re-implementing an identical function.

nevillehuang commented 9 months ago

Invalid, only vesting contract with recipient is intended to cast vote via vesting contract, this is expected behavior, given OZAdaptor holds no voting power

sherlock-admin2 commented 9 months ago

1 comment(s) were left on this issue during the judging contest.

pratraut commented:

'invalid due to voting is only supposed to be happened via VestingEscrow contract'

sherlock-audit / 2024-01-rio-vesting-escrow-judging

0xhashiman - Bad Implementation of Vote Casting in OZVotingAdaptor #40