veRWA - bin2chen's results

Lines of code

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

Vulnerability details

Impact

Adding a new gauge without setting the default time_weight[gauge] may cause subsequent points_weight[gauge][t] to be 0

Proof of Concept

in GaugeController.add_gauge(), Just set isValidGauge[gauge] to true, and didn't modify time_weight[gauge] time_weight[gauge] always defaults to 0

    function add_gauge(address _gauge) external onlyGovernance {
        require(!isValidGauge[_gauge], "Gauge already exists");
@>      isValidGauge[_gauge] = true;
        emit NewGauge(_gauge);
    }

When time_weight[gauge] is 0, it will cause subsequent epochs to not be calculated properly

    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;
        }
    }

Here is the test case. After the second week weight becomes 0

add to GaugeController.t.sol

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

        vm.deal(user1, 1010 ether);

        uint256 lockStart = block.timestamp;
        vm.prank(user1);
        ve.createLock{value: 1000 ether}(1000 ether);

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000);
        assertEq(gc.vote_user_power(user1), 10000);
        vm.stopPrank();
        skip(WEEK); //1 week laster
        gc.checkpoint_gauge(gauge1);
        uint256 _rel_weigth_next = gc.gauge_relative_weight(gauge1, block.timestamp);
        console.log("next week weight:",_rel_weigth_next);        

        skip(WEEK); //2 week laster
        utils.mineBlocks(1);
        gc.checkpoint_gauge(gauge1);
        uint256 _rel_weigth_two_week = gc.gauge_relative_weight(gauge1, block.timestamp);
        console.log("two week weight:",_rel_weigth_two_week);
    }

$ forge test --match-test testNewGuage -vvv

Running 1 test for src/test/GaugeController.t.sol:GaugeControllerTest
[PASS] testNewGuage() (gas: 756546)
Logs:
  next week weight: 1000000000000000000
  two week weight: 0

Tools Used

Recommended Mitigation Steps

init time_weight[gauge]

    function add_gauge(address _gauge) external onlyGovernance {
        require(!isValidGauge[_gauge], "Gauge already exists");
        isValidGauge[_gauge] = true;
+       _change_gauge_weight(_gauge, 0);
        emit NewGauge(_gauge);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

withdraw() if a delegatee set up, it cannot withdraw after the expiration

Proof of Concept

Currently withdraw() requires the delegatee to be yourself

    function withdraw() external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
@>      require(locked_.end <= block.timestamp, "Lock not expired");
@>      require(locked_.delegatee == msg.sender, "Lock delegated");
...        

So if you have a delegatee set up, you need to change it to yourself first but setting it to yourself restricts toLocked can't be expired.

    function delegate(address _addr) external nonReentrant {
...

        require(toLocked.amount > 0, "Delegatee has no lock");
 @>     require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }    

Trying to increase the expiration time via increaseAmount() doesn't work either, again with limitations.

    // @dev A lock is active until both lock.amount==0 and lock.end<=block.timestamp
    function increaseAmount(uint256 _value) external payable nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(_value > 0, "Only non zero amount");
        require(msg.value == _value, "Invalid value");
        require(locked_.amount > 0, "No lock");
@>      require(locked_.end > block.timestamp, "Lock expired");
...        

This leads to the fact that if the user has a delegatee and it expires, it cannot be withdraw. It's locked in the contract.

Tools Used

Recommended Mitigation Steps

Add mechanism to grow lock time

Assessed type

Context