veRWA - Eeyore's results

Lines of code

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

Vulnerability details

In the vote_for_gauge_weights function, users can vote to allocate their voting power towards a specific gauge by specifying its address and the weight of their vote. The function contains a check to ensure that the gauge being voted for is valid via the isValidGauge mapping.

However, there's an oversight. If a gauge gets delisted (i.e., removed from the valid gauges list), any user who had previously allocated voting power to this gauge cannot reassign their voting power elsewhere. This is because the function will revert when attempting to set the weight to any value, including 0, for an invalid gauge.

Impact

Users who have allocated voting power towards a gauge which later gets delisted are locked out from reallocating their voting power. This means they lose their influence in future voting events.

Proof of Concept

1. The governance entity adds two gauges, gauge1 and gauge2.
2. A user (user1) creates a lock and votes for both gauges with a weight of 5000 each. Their total voting power becomes 10000.
3. Governance decides to remove gauge2.
4. When user1 attempts to reassign their voting power from the removed gauge2, they're met with a revert due to "Invalid gauge address". Their voting power is effectively stuck.

PoC Test

Add test to src/test/GaugeController.t.sol and run forge test --match-test testMultiVoteWithRemovedGauge.

function testMultiVoteWithRemovedGauge() public {
    vm.startPrank(gov);
    gc.add_gauge(gauge1);
    gc.add_gauge(gauge2);
    vm.stopPrank();

    vm.startPrank(user1);
    ve.createLock{value: 1 ether}(1 ether);
    gc.vote_for_gauge_weights(gauge1, 5000);
    gc.vote_for_gauge_weights(gauge2, 5000);
    vm.stopPrank();

    assertEq(gc.vote_user_power(user1), 10000);
    uint256 power;
    (,power,) = gc.vote_user_slopes(user1, gauge1);
    assertEq(power, 5000);
    (,power,) = gc.vote_user_slopes(user1, gauge2);
    assertEq(power, 5000);
        
    vm.startPrank(gov);
    gc.remove_gauge(gauge2);
    vm.stopPrank();

    vm.startPrank(user1);
    // should revert when gauge removed and weight is not 0
    vm.expectRevert("Invalid gauge address");
    gc.vote_for_gauge_weights(gauge2, 2000);

    // should allow to remove power from removed gauge when weight is 0 and power > 0
    gc.vote_for_gauge_weights(gauge2, 0);
        
    // should revert when gauge removed and weight is 0
    vm.expectRevert("Invalid gauge address");
    gc.vote_for_gauge_weights(gauge2, 0);

    // should be able to allocate released power
    gc.vote_for_gauge_weights(gauge1, 7000);
    vm.stopPrank();

    assertEq(gc.vote_user_power(user1), 7000);
    (,power,) = gc.vote_user_slopes(user1, gauge1);
    assertEq(power, 7000);
    (,power,) = gc.vote_user_slopes(user1, gauge2);
    assertEq(power, 0);
}

Recommended Mitigation Steps

To address this issue, the function's require statement should allow users to set their weight to 0 even if the gauge address is no longer valid:

- require(isValidGauge[_gauge_addr], "Invalid gauge address");
+ require(isValidGauge[_gauge_addr] || (_user_weight == 0 && vote_user_slopes[msg.sender][_gauge_addr].power > 0), "Invalid gauge address");

Assessed type

Invalid Validation

[L-01] Potential DoS in the VotingEscrow#increaseAmount from expired delegatee lock

Description

In the increaseAmount function, if a user's lock is delegated to a delegatee whose lock has expired, the function will revert. This prevents the user from increasing their lock amount until they change their delegatee.

Recommendation

Provide a mechanism to allow users to increase their lock amount even if their delegatee's lock has expired. This can be done by automatically undelegating the user's lock back to the user.

[L-02] Incorrect unlock time in the Deposit event

Description

In the increaseAmount function, the Deposit event is emitted with the incorrect unlockTime for the user. The event uses the old unlock time instead of the new one.

Recommendation

Ensure that the Deposit event emits the correct unlock time.

- uint256 unlockTime = locked_.end;
LockAction action = LockAction.INCREASE_AMOUNT;
LockedBalance memory newLocked = _copyLock(locked_);
newLocked.amount += int128(int256(_value));
newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);
+ uint256 unlockTime = newLocked.end;

[L-03] Missing check for valid gauge address in the GaugeController#change_gauge_weight

Description

In the change_gauge_weight function, there's no check to ensure that the provided gauge address is valid before changing its weight.

Recommendation

Add a require statement to check if the provided _gauge address is a valid gauge. This will prevent potential misuse or errors.

function change_gauge_weight(address _gauge, uint256 _weight) public onlyGovernance {
+     require(isValidGauge[_gauge], "Invalid gauge address");

[N-01] Inconsistent naming conventions

Description

Throughout the contracts, there are variable and function names that are more reminiscent of Vyper naming conventions (e.g., unlock_time, uepoch) rather than the typical camelCase naming convention in Solidity.

Recommendation

To maintain consistency and readability, rename these variables and functions to follow the camelCase convention.

For example:

- uint256 unlock_time = ...
+ uint256 unlockTime = ...

- function vote_for_gauge_weights(...)
+ function voteForGaugeWeights(...)

[N-02] Within the LockAction enum, the INCREASE_TIME and QUIT are not used

Description

In the VotingEscrow contract, the LockAction enum has two values INCREASE_TIME and QUIT which are not being used in the contract. These are just remnants of an earlier version of the contract.

Recommendation

Remove unused enum values.

enum LockAction {
    CREATE,
    INCREASE_AMOUNT,
    INCREASE_AMOUNT_AND_DELEGATION,
-   INCREASE_TIME,
    WITHDRAW,
-   QUIT,
    DELEGATE,
    UNDELEGATE
}

[N-03] Missing IVotingEscrow interface with documentation about its contents

Description

The contract has comments indicating a reference to the IVotingEscrow interface, but this interface is not provided in the given code. This makes it challenging to understand the expected behavior and interface of the VotingEscrow contract.

Recommendation

Ensure that the IVotingEscrow interface is included when sharing or deploying the contract. This aids in understanding the expected methods and their signatures.

[N-04] Incorrect NatSpec comment use

Description

Throughout the VotingEscrow contract, single-line comments using // are used for function documentation instead of the appropriate NatSpec format using /// or /** */.

Using the correct format is important for generating the contract's interface specification and documentation, ensuring clarity and consistency.

Recommendation

Replace single-line comments // with the appropriate NatSpec format /// for single-line comments or /** */ for multi-line comments before function definitions.

For instance:

- // @dev Floors a timestamp to the nearest weekly increment
+ /// @dev Floors a timestamp to the nearest weekly increment

Similarly, make similar adjustments throughout the contract to conform to the NatSpec format.

[N-05] Redundant check for non-negative user weight in GaugeController#vote_for_gauge_weights

Description

In the vote_for_gauge_weights function, a check is present to ensure the _user_weight is non-negative. However, _user_weight is of type uint256, which is always non-negative.

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

Recommendation

The check _user_weight >= 0 is redundant and can be safely removed.

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