veRWA - thekmj's results

Lines of code

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

Vulnerability details

Summary

If governance removes a gauge for any (non-malicious) reason, a user's voting power for that gauge will be completely lost.

Vulnerability details

The GaugeController is a solidity port of Curve DAO's Vyper implementation. Users are to vote for channeling incentives by using the vote_for_gauge_weights() function, and each user can fraction their voting power by $10000$ (that is, defined by BPS).

One modification from the original is that governance can now remove gauges, not allowing users to vote on it. However, any existing individual user's voting power before removal is not reset. Since vote_for_gauge_weights() does not allow voting for removed gauges, the voting power is then forever lost.

Consider the following scenario:

• Alice has some veRWA, and is now able to vote.
• She votes on some pool, say, G1, using 100% of her voting power.
• Pool G1 is removed by governance due to any reason. Perhaps the pool was found to be faulty and liquidity should be migrated, perhaps the market itself has became illiquid and unsafe, perhaps the intended incentives duration for that pool has simply expired.
• Alice still has 100% of her voting power in that pool, but she cannot remove her vote and claim the voting power back.

It is worth noting that, even if Alice does not use 100% of her voting power on that particular gauge, she would still lose whatever percent vote placed in that pool, and her overall voting power was weakened by said percent.

Impact

Users can lose their voting power.

Proof of concept

We provide the following POC to use on GaugeController tests.

function testPOC() public {
    // prepare
    uint256 v = 10 ether;
    vm.deal(gov, v);
    vm.startPrank(gov);
    ve.createLock{value: v}(v);

    // add gauges
    gc.add_gauge(gauge1);
    gc.change_gauge_weight(gauge1, 100);
    gc.add_gauge(gauge2);
    gc.change_gauge_weight(gauge2, 100);

    // all-in on gauge1
    gc.vote_for_gauge_weights(gauge1, 10000);

    // governance removes gauge1
    gc.remove_gauge(gauge1);

    // cannot vote for gauge2
    vm.expectRevert("Used too much power");
    gc.vote_for_gauge_weights(gauge2, 10000);

    // cannot remove vote for gauge1
    vm.expectRevert("Invalid gauge address"); // @audit remove when mitigate
    gc.vote_for_gauge_weights(gauge1, 0);

    // cannot vote for gauge2 (to demonstrate again that voting power is not removed)
    vm.expectRevert("Used too much power");  // @audit remove when mitigate
    gc.vote_for_gauge_weights(gauge2, 10000);
}

Tools used

Forge, manual review

Recommended mitigation steps

The simplest way to mitigate this is to allow zero-weight votings on expired pools simply to remove the vote. Modify line 213 as follow:

require(_user_weight == 0 || isValidGauge[_gauge_addr], "Can only vote 0 on non-gauges");

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

The given POC can then be the test case to verify successful mitigation.

As a QA-based recommendation, the sponsor can also provide an external function to remove votes, and/or provide a function to vote for various pools in the same tx. This will allow users to channel their votes directly from removed pools to ongoing pools.

Assessed type

Invalid Validation

[L-01] Hardcoding lock duration to 5 years may discourage users from locking.

The original veCRV implementation allows users to lock CRV tokens in exchange for voting power. Each CRV token locked for the maximum duration will yield one unit of voting power, and said voting power decays with time. The same idea is applied here, but with CANTO.

However, while veCRV allows users to specify the lock duration, veRWA does not, and only allows unlocking after 5 years of inactivity. This removes a certain degree of flexibility in the economy game, and may discourage users from locking their tokens, due to the high risk nature of smart contracts and cryptocurrency market.

[L-02] Documentation do not properly reflect the changes from the original VotingEscrow.

There is no such file IVotingEscrow present in the repository.

• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L267
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L286
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L325
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L325
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L472
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L486
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L564
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L571

One should not be relying on FIAT DAO's original IVotingEscrow as the documented source of truth, as the contract veRWA has undergone changes. Several behaviors are important to document for the user to be aware of. For example, increaseAmount also resets their lock to 5 years, or delegate actually does not reset the lock (contrary to the statement at line 18).

[L-03] Events should be emitted for important events

Because changes in voting are equivalent to changes in incentives, it is recommended to emit event for any gauge-related state changes. It will be beneficial to whoever user looking to monitor incentives and its historical data.

We recommend emitting events for vote_for_gauge_weights() whenever a user makes a vote, and _change_gauge_weight for whenever governance changes a pool weight.

Affected functions:

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

[N-01] Redundant equivalence check for msg.value

It is enough to use msg.value itself as the lock value. There is no need to require it as a function parameter.

• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L18
• https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L292

[N-02] Voting token metadata should either be all hardcoded or all immutable

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

In VotingEscrow, the voting token metadata are set to storage.

However, it is known that they will be the native CANTO token, with known immutable metadata.

We suggest setting all of them to a constant, or to immutables. This also has the virtue of gas-saving by reducing storage reads whenever the data are used in a transaction (e.g. decimals).

[N-03] Redundant limit check for voting power

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

In the function vote_for_gauge_weights(), there is an assertion for user voting weight being in range $[0, 10000]$.

require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");

However, if the user attempts to input more than $10000$, then the call will revert anyway from the following assertion.

require(power_used >= 0 && power_used <= 10_000, "Used too much power");

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

[N-04] Incorrect variable addresses on testing

In the test file GaugeController.t.sol, user1 is often used as a gauge instead of gauge1. Using these two variables interchangeably may lead to confusions, especially in future integrations. Thus it is not recommended.

[R-01] Recommend allowing users to vote for multiple gauges in one transaction

Currently, to edit a vote for a gauge, a user must send one transaction. This means if the user wants to divide their votes across gauges (e.g. if they have multiple positions in multiple pools), then they'd have to send multiple transactions.

We suggest exposing a function for users to update their vote for multiple pools. Validate that the user's total vote after the transaction is less than $10000$ is enough.

Summary of the project

The project implements a veToken voting-escrow model, similar to veCRV. Users are able to lock their CANTO tokens in exchange for voting power. Users can then vote to channel incentives to different pools, e.g. for pools they have positions in.

The project is a fork of various protocols, with changes.

• VotingEscrow: Fork of FIAT DAO's implementation.
• GaugeController: Solidity implementation of Curve's Vyper counterpart.
• LendingLedger: For distributing rewards. Is not a fork.

For the forked contracts, the major changes are as follow:

• VotingEscrow now uses native token, instead of ERC20 token.
• In FIAT DAO's Voting Escrow, a user is able to specify their lock duration (not exceeding MAX and ending at valid timestamps). In veRWA, the lock duration is fixed at 5 years (maximum possible).
• GaugeController: Governance is able to remove gauges.
  • Additionally, governance does not have to specify gauge weight upon addition.

Approach taken in evaluating the codebase

For the forked contracts, we focus on the changes made, and its implications on existing variables of the contract. Thus we focus mostly on the functionalities and algorithmic aspects. We do not focus on the mathematical specifics, as the math workload are mostly forked, and has already been battle-tested.

For LendingLedger however, we do focus on the specifics. However, it is still more algorithmic, rather than mathematical, in nature.

We identified and have submitted the following issues and recommendations within the codebase:

Centralization risks

There are two contracts holding funds:

• VotingEscrow: Holds locked CANTO
• LendingLedger: Holds rewards.

In both contracts, we identified no functions for the admin to withdraw funds to themselves.

However, governance is able to override gauge weights using the function GaugeController.change_gauge_weight(). This is technically a possible path to manipulate rewards distribution.

Governance also has the responsibility to correctly distribute rewards for every epoch (every week), as well as ensuring enough CANTO is locked in the reward Ledger.

Systemic risks

External outgoing calls happen only when sending CANTO to the users, either through withdraw() or claim(). Both functions either have re-entry guards in place, or follows the CEI pattern.

However, external incoming call also happens in sync_ledger(). While only whitelisted markets can call this function, it can still affect rewards distribution. It is then the team's responsibility to diligently verify the market was correctly implemented before whitelisting.

Architecture recommendations

LendingLedger currently only supports rewarding in CANTO/native token. We suggest integrating ERC20 tokens into the mix, as this will enable external partnered protocols to provide incentives, which may boost the growth of the CANTO ecosystem.

Time spent:

16 hours