sherlock-admin2 commented 9 months ago

Shaheen

high

Voting Escrow Recipient can increase his voting power or regain his voting power after claiming all the vested tokens

Summary

Recipients can increase their voting power or regain their voting power after claiming all the vested tokens

Vulnerability Detail

The Vesting Escrow contract allows recipients to receive vesting tokens and based on that tokens it gives recipients the voting power. When the recipient claims all the vested tokens, the voting power of the user becomes zero. And the recipient no longer can participate in the governance.

The problem is that the voting power depends on the contract balance. As written in the VestingEscrow#vote comment; "Participate in a governance vote using all available tokens on the contract's balance."

The recipient/user can easily increase the contract balance by sending ERC20 tokens directly to the contract. By sending the tokens, contract balance will increase, when contract balance will increase, user's voting power/balance will increase too. This can be done to regain or massively increase the voting power.

There is also a recoverERC20 function also present which allows the recipient/user to withdraw any ERC20 tokens (except for unclaim+locked). That's why user can deposit & withdraw freely, without risking of freezing his funds.

Also, a user might get revokeAll() due to some legal documentation, then the user can get back his voting rights by exploiting this vulnareblity. As revokeAll() function is in place so the org deploying the vesting escrow can clawback all the tokens, when something written into a legal contract surpasses. If the user breaks any legal conditions or do something malicious with the voting power, the owner of the contract will call revokeAll() which will clawback locked + unclaimed tokens and users' voting power will become zero.

Proof-of-Concept

Add this test into the VestingEsrow.t.sol & run forge test --mt testIncreaseVotingPowerOrRegainVotingPowerAfterClaimAll_PoC -vv

   function testIncreaseVotingPowerOrRegainVotingPowerAfterClaimAll_PoC() public {
        vm.warp(endTime);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(deployedVesting)));

        console.log("voting power before claim", token.getVotes(address(deployedVesting)));
        assertEq(token.getVotes(address(deployedVesting)), token.balanceOf(address(deployedVesting)));

        vm.prank(recipient);
        deployedVesting.claim(recipient, type(uint256).max);

        //> Voting power = Zero, after claim
        assertEq(token.getVotes(address(deployedVesting)), 0);
        console.log("voting power after claim", token.getVotes(address(deployedVesting)));

        vm.prank(recipient);
        deal(address(token), recipient, 5 ether);
        token.transfer(address(deployedVesting), 5 ether);
        console.log("voting power increased", token.getVotes(address(deployedVesting)));
        //> Voting power increases after direct tokens transfer to the vesting contract
        assertEq(token.getVotes(address(deployedVesting)), token.balanceOf(address(deployedVesting)));
        assertEq(token.balanceOf(address(deployedVesting)), 5 ether);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(deployedVesting)));

        uint256 proposalId = createProposal();
        uint256 votingBalance = token.getVotes(address(deployedVesting));

        vm.roll(block.number + 1);

        vm.prank(recipient);
        deployedVesting.vote(ozVotingAdaptor.encodeVoteCallData(proposalId, uint8(VoteType.For)));

        (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) = governor.proposalVotes(proposalId);
        assertEq(forVotes, votingBalance);
        assertEq(againstVotes, 0);
        assertEq(abstainVotes, 0);
    }

Logs:

Running 1 test for test/VestingEscrow.t.sol:VestingEscrowTest
[PASS] testIncreaseVotingPowerOrRegainVotingPowerAfterClaimAll_PoC() (gas: 439443)
Logs:
  voting power before claim 1000000000000000000
  voting power after claim  0
  voting power increased    5000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.89ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Governance Compromised This will compromise the Governance, as users can increase their voting power (as much as they want), more voting power then they were initially assigned to. Or at least regain their voting power whenever they want (after claim all). And a malicous user can get voting rights even after getting revoked.

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/2d14c2b84b69c53a45c81aa4f907af9617f9a94f/rio-vesting-escrow/src/VestingEscrow.sol#L152

Tool used

🦅

Recommendation

To completely mitigate this issue, The voting power should not be dependent on the contract balance. To make this vulnerability hard to exploit, Remove recoverERC20.

I encourage the Rio team to discover better ways to handle this issue. Thanks!

nevillehuang commented 9 months ago

Invalid, voting power is detrmined by the balance of tokens of vesting contract voting. Once recipient has claimed tokens after vesting period ends and directly transfer tokens to vesting contract, the contract will have the same voting power as when he has vested his tokens, so there is no issue here.

sherlock-admin2 commented 9 months ago

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

pratraut commented:

'invalid as test case is failing when changing vm.prank to vm.startPrank for delegate call'

TradMod commented 9 months ago

escalate

@nevillehuang sir, isn't that's an issue it-self? especially when we consider revokeAll functionality as well

voting power is detrmined by the balance of tokens of vesting contract voting. Once recipient has claimed tokens after vesting period ends and directly transfer tokens to vesting contract, the contract will have the same voting power as when he has vested his tokens, so there is no issue here.

revokeAll can also be used when the recipient breaks some legal contracts/documentation, that means user does something malicious. A malicious user having voting power doesn't seems good. Consider a legal documentation that doesn't allow certain users to have voting power after some certain timeframe and owner uses the revokeAll to make users' voting power zero but the users regain their voting power using this loophole. fcxtrdxtdx

Sponsor said in the private thread:

this is a token vesting contract that's likely backed by some sort of conditions in a legal contract. if any of those conditions are broken (no longer employed, etc) (the revokeAll will be used.....)

This vulnerability can be used by a revoked user to gain back his voting power, that what it makes a Medium issue maybe not high as revoke will not be used for that purpose much oftenly.

Pls sponsor @solimander & judge @nevillehuang consider reviewing this issue again. Thanks!

sherlock-admin2 commented 9 months ago

escalate

@nevillehuang sir, isn't that's an issue it-self? especially when we consider revokeAll functionality as well

voting power is detrmined by the balance of tokens of vesting contract voting. Once recipient has claimed tokens after vesting period ends and directly transfer tokens to vesting contract, the contract will have the same voting power as when he has vested his tokens, so there is no issue here.

revokeAll can also be used when the recipient breaks some legal contracts/documentation, that means user does something malicious. A malicious user having voting power doesn't seems good. Consider a legal documentation that doesn't allow certain users to have voting power after some certain timeframe and owner uses the revokeAll to make users' voting power zero but the users regain their voting power using this loophole.

Sponsor said in the private thread:

this is a token vesting contract that's likely backed by some sort of conditions in a legal contract. if any of those conditions are broken (no longer employed, etc) (the revokeAll will be used.....)

This vulnerability can be used by a revoked user to gain back his voting power, that what it makes a Medium issue maybe not high as revoke will not be used for that purpose much oftenly.

Pls sponsor @solimander & judge @nevillehuang consider reviewing this issue again. Thanks!

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 9 months ago

@solimander Could you take a look at this? My understanding is once recipient claim tokens, they are free to use those tokens however they like, so we cannot deem such voting as "malicious".

And if they directly donate this funds to the contract, will they permanently lose this funds based on revokeAll()/recoverERC20() logic?

TradMod commented 9 months ago

My understanding is once recipient claim tokens, they are free to use those tokens however they like, so we cannot deem such voting as "malicious".

@nevillehuang Sir I also raised concerns around a malicious user, who gets revoked and then he regains his power using this loophole, both in my submission and the escalation. That's why I believe this should be atleast a medium severity issue.

if they directly donate this funds to the contract, will they permanently lose this funds based on revokeAll()/recoverERC20() logic?

No, they will not lose funds. If a user donates funds to the contract directly, he can recover them easily using recoverERC20(). revokeAll() transfers unclaimed + unvested tokens to the owner only recoverERC20() transfers all the balance to the recipient except unclaimed + unvested tokens

Just to be clear, I want to present my case again of the malicious revoked user:

A user breaks some legal contract or uses his voting power maliciously
Protocol decides to revoke that user
Protocol revokes that user and the user now have zero voting power
Malicious user finds that the voting power depends on the contract balance
User donates tokens to the contract to regain voting power

Note: This problem (revoked User) is not in the Lido contracts because they have deployed contracts with default isFullyRevokable == false. And they are solving the problem, by which user can get more voting power than they were initially assigned to, by using a snapshot system. So that's a problem itself as well & and that makes it a high severity issue.

Hope this helps. Thanks!

PoC:

    function testRegainVotingPowerAfterRevokeAll_PoC() public {
        vm.warp(endTime);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(deployedVesting)));

        console.log("voting power before revoke", token.getVotes(address(deployedVesting)));
        assertEq(token.getVotes(address(deployedVesting)), token.balanceOf(address(deployedVesting)));

        vm.prank(factory.owner());
        deployedVesting.revokeAll();

        //> Voting power = Zero, after revoke
        assertEq(token.getVotes(address(deployedVesting)), 0);
        console.log("voting power after revoke ", token.getVotes(address(deployedVesting)));

        vm.prank(recipient);
        deal(address(token), recipient, 1 ether);
        token.transfer(address(deployedVesting), 1 ether);
        console.log("user regains voting power ", token.getVotes(address(deployedVesting)));
        //> Voting power increases after direct tokens transfer to the vesting contract
        assertEq(token.getVotes(address(deployedVesting)), token.balanceOf(address(deployedVesting)));
        assertEq(token.balanceOf(address(deployedVesting)), 1 ether);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(deployedVesting)));

        uint256 proposalId = createProposal();
        uint256 votingBalance = token.getVotes(address(deployedVesting));

        vm.roll(block.number + 1);

        vm.prank(recipient);
        deployedVesting.vote(ozVotingAdaptor.encodeVoteCallData(proposalId, uint8(VoteType.For)));

        (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) = governor.proposalVotes(proposalId);
        assertEq(forVotes, votingBalance);
        assertEq(againstVotes, 0);
        assertEq(abstainVotes, 0);
    }

Running 1 test for test/VestingEscrow.t.sol:VestingEscrowTest 
[PASS] testRegainVotingPowerAfterRevokeAll_PoC() (gas: 432326)
Logs:
  voting power before revoke 1000000000000000000
  voting power after revoke  0
  user regains voting power  1000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.29ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

zzykxx commented 9 months ago

I don't see why this should be considered valid. The purpose of the revokeAll() function is to prevent the recipient from withdrawing his vested (and unclaimed) tokens.

If I'm a recipient of a revoked vesting escrow with some tokens I can simply go vote on the governance directly without having to exploit this "loophole".

TradMod commented 9 months ago

@zzykxx, thanks for your comment sir, sorry I didn't quite understand your words fully

If I'm a recipient of a revoked vesting escrow with some tokens I can simply go vote on the governance directly without having to exploit this "loophole".

isn't the tokens have voting power only when they are in the voting escrow contract? That's why when user claims the tokens his voting power become zero: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/2d14c2b84b69c53a45c81aa4f907af9617f9a94f/rio-vesting-escrow/test/VestingEscrow.t.sol#L229

zzykxx commented 9 months ago

@zzykxx, thanks for your comment sir, sorry I didn't quite understand your words fully

If I'm a recipient of a revoked vesting escrow with some tokens I can simply go vote on the governance directly without having to exploit this "loophole".

isn't the tokens have voting power only when they are in the voting escrow contract? That's why when user claims the tokens his voting power become zero: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/2d14c2b84b69c53a45c81aa4f907af9617f9a94f/rio-vesting-escrow/test/VestingEscrow.t.sol#L229

As far as I understand tokens have voting power also outside the escrow, the escrow is simply a way to allow recipients to vote while their tokens are locked. I try to rephrase my explanation above in a better way (sorry that was not really clear):

Let's suppose I'm the recipient of a voting escrow in which 1000 tokens ABC are locked and I also own 500 ABC tokens outside the escrow. If the owners decide to call revokeAll() and revoke all of the 1000 ABC tokens my voting power from the voting escrow will be 0 because there are zero tokens there. But regardless of what happens to the voting escrow I can still use my 500 ABC tokens to vote on the governance directly. There is no need to use the approach you described in the issue.

I'm not entirely sure though, so let's wait for @nevillehuang and @solimander.

TradMod commented 9 months ago

@zzykxx I see, thanks for the clarification sir. That makes sense but I just ran a simple test and looks like thats not how it works. It still looks to me that the voting power is attached with the escrow contract only. Yeah, If @solimander sir confirms what you've said then I'll accept that there is no issue.

EDIT: Also, if what you've said is correct then why the protocol expects the user to have zero voting power after claim? https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/2d14c2b84b69c53a45c81aa4f907af9617f9a94f/rio-vesting-escrow/test/VestingEscrow.t.sol#L244

Btw this is the stupid test I quickly wrote to confirm what you said (pls take a look and correct me if I'm doing something wrong)

    function testVotesOfTheUser() public {
        vm.warp(endTime);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(recipient)));
        console.log("voting power user before claim", token.getVotes(address(recipient)));

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(deployedVesting)));
        console.log("voting power before claim", token.getVotes(address(deployedVesting)));

        assertEq(token.getVotes(address(deployedVesting)), token.balanceOf(address(deployedVesting)));

        vm.prank(recipient);
        deployedVesting.claim(recipient, type(uint256).max);

        assertEq(token.getVotes(address(deployedVesting)), 0);

        vm.prank(recipient);
        deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(address(recipient)));
        console.log("voting power user after claim", token.getVotes(address(recipient)));
        console.log("voting power after claim ", token.getVotes(address(deployedVesting)));
    }

Running 1 test for test/VestingEscrow.t.sol:VestingEscrowTest
[PASS] testVotesOfTheUser() (gas: 267989)
Logs:
  voting power user before claim 1000000000000000000
  voting power before claim 1000000000000000000
  voting power user after claim 0
  voting power after claim  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.42ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

solimander commented 9 months ago

@ShaheenRehman The voting power is not attached to the escrow. If a user has tokens outside the escrow, they do not gain any voting power by depositing them into the escrow.

The key point that's misunderstood here is that the token balance does not account for voting power by default in ERC20Votes. A token holder must delegate to themselves in order to activate checkpoints and have their voting power tracked.

i.e. the recipient must delegate to themselves (or someone else) to "activate" their voting power:

function testVotesOfTheUser() public {
    vm.warp(endTime);

    vm.prank(recipient);
    deployedVesting.claim(recipient, type(uint256).max);
    assertEq(token.getVotes(address(deployedVesting)), 0);

    vm.prank(recipient);
    token.delegate(address(recipient));
    console2.log("voting power user after claim", token.getVotes(address(recipient)));
}

TradMod commented 9 months ago

Ah I see, Thanks for the clarification sir, I got it now. It's interesting how it works!

TLDR of the discussion for the Judge: Invalid issue

Running 1 test for test/VestingEscrow.t.sol:VestingEscrowTest
[PASS] testVotesOfTheUser() (gas: 159176)
Logs:
  voting power user after claim 1000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 60.33ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Czar102 commented 8 months ago

Result: Invalid Unique

sherlock-admin commented 8 months ago

Escalations have been resolved successfully!

Escalation status:

ShaheenRehman: rejected

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

Shaheen - Voting Escrow Recipient can increase his voting power or regain his voting power after claiming all the vested tokens #57

Voting Escrow Recipient can increase his voting power or regain his voting power after claiming all the vested tokens

Summary

Vulnerability Detail

Proof-of-Concept

Impact

Code Snippet

Tool used

Recommendation