search

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

3 stars 2 forks source link

itsabinashb - VestingEscrow::user can maliciously increase voting power of delegatee #51

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 9 months ago

itsabinashb

high

VestingEscrow::user can maliciously increase voting power of delegatee

Summary

An user can call delegate() of VestingEscrow.sol contract and increase voting power of a delegatee as much as they want.

Vulnerability Detail

The deployVestingContract() of VestingEscrowFactory.sol contract allows a user to call it multiple time, but the thing is each time the data will be updated with latest data which is passed as argument in deployVestingContract(). The delegate() also allows same user to call it multiple times, it has no check whether the same delegatee is passed, whether the same user calling it etc. As a result any user can start vesting as many as possible by calling the deployVestingContract() and then call the delegate() to increase voting power of a particular delegatee. We can verify this case by a test, to conduct the test we need to comment out 2 lines from TestUtil.sol contract - this one and this one. Now create a test file inside test folder and paste this test:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {TestUtil} from 'test/lib/TestUtil.sol';
import {ERC20Token} from 'test/lib/ERC20Token.sol';
import 'forge-std/console.sol';

contract Audit is TestUtil {
  address owner = vm.addr(1);
  address manager = vm.addr(2);
  address user1 = vm.addr(3);
  address delegatee = vm.addr(9);

  function setUp() external {
    setUpProtocol(ProtocolConfig(owner, manager));
  }

  function test_delegateMultipleTimeByVestingMultipleTime() external {
    vm.startPrank(user1);
    deployVestingEscrow(VestingEscrowConfig(1 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(delegatee));
    deployVestingEscrow(VestingEscrowConfig(1 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(delegatee));
    deployVestingEscrow(VestingEscrowConfig(1 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(delegatee));
    deployVestingEscrow(VestingEscrowConfig(1 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(delegatee));
    deployVestingEscrow(VestingEscrowConfig(1 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    deployedVesting.delegate(ozVotingAdaptor.encodeDelegateCallData(delegatee));
    console.log("voting power of delegatee:", token.getVotes(delegatee));
  } 

}

The result of the test is:

[PASS] test_delegateMultipleTimeByVestingMultipleTime() (gas: 1185512)
Logs:
  voting power of delegatee: 5000000000000000000

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

You can see how a user could manage to increase voting power of the delegatee.

Impact

User can increase voting power of a delegatee as much as he want.

Code Snippet

  1. https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L148
  2. https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L268
  3. https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L51

    Tool used

Manual Review, Foundry

Recommendation

An user should not delegate a delegatee more than 1 time.

solimander commented 9 months ago

Invalid. Governance tokens holders can always choose to delegate to the same address if they like.

itsabinashb commented 9 months ago

But it incresing the voting power by which the delagatee win any vote, is not it unexpected??? The code allowing me to delegate multiple time so I can vest as many time I want and increase the voting power as much as I want, that is what I shown in test, are you sure this behaviour is expected??

solimander commented 9 months ago

The organization that grants the tokens is the one that deposits them into escrow. Regardless, whoever has initial access to the tokens can delegate the full balance to anyone, with or without the vesting contracts.

itsabinashb commented 9 months ago

The organization that grants the tokens is the one that deposits them into escrow. Regardless, whoever has initial access to the tokens can delegate the full balance to anyone, with or without the vesting contracts.

No sir, As per current implementation of contracts organisation does not deposits tokens into escrow, the recipient, who opened vesting position, is the one who deposits token in escrow contract, you can see it here:

FOR User 1

 ├─ [0] VM::startPrank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [96778] OZVotingToken::mint(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 1000000000000000000 [1e18])
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 1000000000000000000 [1e18])
    │   └─ ← ()
    ├─ [24605] OZVotingToken::approve(VestingEscrowFactory: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
    │   ├─ emit Approval(owner: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, spender: VestingEscrowFactory: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
    │   └─ ← true
    ├─ [119023] VestingEscrowFactory::deployVestingContract(1000000000000000000 [1e18], 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 864000 [8.64e5], 1, 432000 [4.32e5], true, 0x)
    │   ├─ [41458] → new <unknown>@0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c
    │   │   └─ ← 207 bytes of code
    │   ├─ [22516] OZVotingToken::transferFrom(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c, 1000000000000000000 [1e18])
    │   │   ├─ emit Transfer(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, to: 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c, value: 1000000000000000000 [1e18])
    │   │   └─ ← true
    │   ├─ [31321] 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c::initialize(true, 0x)
    │   │   ├─ [28590] VestingEscrow::initialize(true, 0x) [delegatecall]
    │   │   │   ├─ [816] OZVotingToken::balanceOf(0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c) [staticcall]
    │   │   │   │   └─ ← 1000000000000000000 [1e18]
    │   │   │   ├─ emit VestingEscrowInitialized(factory: VestingEscrowFactory: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], recipient: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, token: OZVotingToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], amount: 1000000000000000000 [1e18], startTime: 1, endTime: 864001 [8.64e5], cliffLength: 432000 [4.32e5], isFullyRevokable: true)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   ├─ emit VestingEscrowCreated(creator: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, recipient: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, escrow: 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c)
    │   └─ ← 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c
    ├─ [0] VM::stopPrank()
 */

Now see this line:

 │   ├─ [41458] → new <unknown>@0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c
    │   │   └─ ← 207 bytes of code
    │   ├─ [22516] OZVotingToken::transferFrom(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c, 1000000000000000000 [1e18])
    │   │   ├─ emit Transfer(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, to: 0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c, value: 1000000000000000000 [1e18])
    │   │   └─ ← true

0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c is the escrow contract address, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69 is the recipient address.

solimander commented 9 months ago

No sir, As per current implementation of contracts organisation does not deposits tokens into escrow, the recipient, who opened vesting position, is the one who deposits token in escrow contract, you can see it here...

The setup script doesn't make the call as the owner because it's not necessary as there is no access control for convenience, though all deployed vesting escrows are controlled by the owner. These contracts are designed for token grants (like stock grants), which allows the recipient to use their full governance power immediately.

Even if the user wanted to create a vesting escrow using their tokens, they would not gain anything. They'd only lose the ability to transfer the tokens for the vesting period. They already have full control over the voting power of the tokens they hold and could delegate them if they want. The vesting escrow delegate function does not give them any special ability.

nevillehuang commented 9 months ago

Invalid, in this test you are assuming admin deployed 5 different escrows contract for the same user1, so it is intended that delegatee assigned by recipient have 5e18 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 deployer or recipient is a TRUSTED entity'