veRWA - 0xDetermination's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L214-L223 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L68-L284 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L356-L409

Vulnerability details

Impact

A malicious user can arbitrarily increase any gauge's weight, resulting in loss of rewards for lenders of other gauges.

Proof of Concept

Note that the below code snippet from GaugeController.vote_for_gauge_weights() (called by users to cast and change votes) contains the only interactions between GaugeController.sol and VotingEscrow.sol:

VotingEscrow ve = votingEscrow;
        (
            ,
            /*int128 bias*/
            int128 slope_, /*uint256 ts*/

        ) = ve.getLastUserPoint(msg.sender); // get the slope of the user's last recorded Point
        require(slope_ >= 0, "Invalid slope");
        uint256 slope = uint256(uint128(slope_));
        uint256 lock_end = ve.lockEnd(msg.sender); // get user's lock expiry time

Therefore, GaugeController.sol does not know when users lose their voting power in VotingEscrow.sol by calling VotingEscrow.delegate(). Malicious users can abuse this to gain an arbitrary amount of voting power.

Steps to reproduce:

1. User calls VotingEscrow.createLock() and creates a lock.
2. User calls GaugeController.vote_for_gauge_weights() to vote for a gauge.
3. User creates a new lock using a different address, locking an economically negligible amount of value.
4. User calls VotingEscrow.delegate() and delegates the original lock's voting power to the new address.
5. User calls GaugeController.vote_for_gauge_weights() with the new address to vote for a gauge.
6. User repeats steps 3-5 an arbitrary number of times to increase gauge weights by an arbitrary amount.

Tools Used

Manual Review

Recommended Mitigation Steps

This may be somewhat difficult to fix due to the way GaugeController.sol stores votes and interacts with VotingEscrow.sol. A possible mitigation would be to force users to call VotingEscrow.delegate() through GaugeController.sol, and store delegated slope/bias changes in GaugeController.sol's state so that they can be nullified upon redelegation.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L185-L206 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L145-L182 https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L173-L183 https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L146-L161 https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L124-L132

Vulnerability details

Impact

Users may be given outsized rewards; therefore it is possible for LendingLedger's CANTO balance to drop below the reward amount of a valid user claim, in which case that claim will fail. Furthermore, gauge decay accounting is made inaccurate.

Proof of Concept

Note the below code and inline comments for GaugeController.change_gauge_weight():

    function _change_gauge_weight(address _gauge, uint256 _weight) internal {
        uint256 old_gauge_weight = _get_weight(_gauge); // current weight of the gauge
        uint256 old_sum = _get_sum(); // global weight
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;

        points_weight[_gauge][next_time].bias = _weight; // gauge's weight is set to the _weight argument. For the presented case, _weight is less than the current gauge weight
        time_weight[_gauge] = next_time; // gauge's "time checkpoint" is updated, not relevant for this issue

        uint256 new_sum = old_sum + _weight - old_gauge_weight; // new_sum is less than old_sum for this case
        points_sum[next_time].bias = new_sum; // global bias is updated
        time_sum = next_time; // global "time checkpoint" is updated, not relevant for this issue
    }

    /// @notice Allows governance to overwrite gauge weights
    /// @param _gauge Gauge address
    /// @param _weight New weight
    function change_gauge_weight(address _gauge, uint256 _weight) public onlyGovernance { // all logic is handled by the internal function _change_gauge_weight
        _change_gauge_weight(_gauge, _weight);
    }

We see here that the weight (bias) is updated for both the global state and the individual gauge, while slope is not updated at all. Consider the following: let the equation $f(t) = b - mt$ represent the gauge's decay equation before the weight is reduced by some amount $k$. (Note that $m$, or slope, is always positive in the protocol's code.) After the weight is reduced by calling change_gauge_weight(), the equation becomes $f(t) = -k + b -mt$. The slope is unchanged, but the t-axis intercept has changed from $t_{before} = b/m$ to $t_{after} = (-k + b)/m$, the latter of which is a smaller value. Now, because global "slope changes" (slope values that should be subtracted from the global slope upon their decay equations equaling zero) are stored in the changes_sum timestamp to slope mapping, which is not modified by change_gauge_weight(), there is a time period with length determined by the value of $t_1 - t_2$ during which the previous slope changes applied to the global state by user calls to vote_for_gauge_weights() remain applied even though they should have been subtracted at the start of the aforementioned time period.

This results in the global weight being less than the sum of the weights of all the individual gauges, which is an accounting error. Thus when LendingLedger.claim() calls GaugeController.gauge_relative_weight_write() on the affected time period, the calculated relative weight will be inflated, and LendingLedger.claim() will disburse outsized rewards. If the magnitude of the outsized rewards is large enough, then the LendingLedger contract may lose enough funds to where the claim() function starts to fail. Furthermore, it's also possible for the global weight to reach zero before the weight of all gauges reaches zero, possibly resulting in active gauges receiving zero rewards.

Note that this issue only applies once a gauge's weight has decayed to zero. This is guaranteed to occur at the next epoch if a gauge (having nonzero bias, nonzero slope, and t-intercept greater than the current time plus two weeks) is removed with GaugeController.remove_gauge().

Finally, reducing the weight of a gauge makes its decay equation inaccurate, as can be seen from the t-intercept changes discussed.

Tools Used

Manual Review

Recommended Mitigation Steps

This issue is difficult to fix without losing functionality in change_gauge_weights() due to the way that slope changes are calculated by the contract; the accounting logic in GaugeController may need to be changed significantly. The issue can be easily fixed by not allowing governance to reduce gauge weights.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L124-L132 https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L208-L278

Vulnerability details

Impact

Users will permanently lose voting power equal to their power used to vote in the removed gauge unless the gauge is re-added and users re-vote to remove their voting power from the gauge.

Proof of Concept

GaugeController.sol does not have any function for users to remove their voting power from gauges, apart from vote_for_gauge_weights(). Notice the below code snippets and inline comments:

    function remove_gauge(address _gauge) external onlyGovernance {
        require(isValidGauge[_gauge], "Invalid gauge address");
        isValidGauge[_gauge] = false; // Removing a gauge renders it "invalid"

    function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external {
        require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight"); 
        require(isValidGauge[_gauge_addr], "Invalid gauge address"); // Error is thrown if this function is called on a removed gauge

Because users cannot remove their voting power from a removed gauge, they may indefinitely lose voting power.

Tools Used

Manual Review

Recommended Mitigation Steps

Only require that a valid gauge is passed to vote_for_gauge_weights() if the _user_weight argument is greater than zero. Alternatively, add a function that allows users to remove their votes from any gauge.

Assessed type

Other

QA report for veRWA by 0xDetermination

[L-01] LendingLedger.claim() should use nonReentrant modifier

Because claim() contains an external call, the nonReentrant modifier should be used. (Note that VotingEscrow.withdraw() uses the nonReentrant modifier.)

Recommendations

Add the nonReentrant modifier to claim().

[L-02] LendingLedger.sol can permanently lock up CANTO

Note that LendingLedger.sol is not meant to receive funds from anywhere except the reward distributor, and its receive() function is empty. Furthermore, the only way to withdraw funds from the contract is the claim function, which gives out appropriate rewards to users. Therefore, any funds sent to LendingLedger.sol that are not from the reward distributor will remain permanently locked in the contract.

Recommendations

receive() should revert if funds are sent from an address that is not the reward distributor.

[NC-01] "epoch" in VotingEscrow.sol does not refer to the same concept as "epoch" in the other two contracts

//Give Examples, elaborate on different "epoch" concept Notice the code snippet from GaugeController.sol below (line 60):

        uint256 last_epoch = (block.timestamp / WEEK) * WEEK;

And this snippet from LendingLedger.sol (also on line 60):

        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;

Both of these snippets indicate that an "epoch" is a period lasting one week. Now, note the below snippet from VotingEscrow.sol (line 146):

            userPointEpoch[_addr] = uEpoch + 1;

This line runs in the _checkpoint() function and is executed every time a user calls createLock(), increaseAmount(), withdraw(), or delegate(). Because a user is not restricted to calling these functions once a week, "epoch" in VotingEscrow.sol cannot refer to a period of one week. Similarly, VotingEscrow.globalEpoch is incremented every time _checkpoint() is called. See the below code snippet (lines 211-220):

            //The below code runs every time _checkpoint() is called.
            epoch = epoch + 1; //epoch was instantiated to globalEpoch, now it's incremented
            if (iterativeTime == block.timestamp) {
                lastPoint.blk = block.number;
                break;
            } else {
                pointHistory[epoch] = lastPoint;
            }
        }

        globalEpoch = epoch; //globalEpoch is incremented by 1

The difference in the meaning of "epoch" between the contracts is confusing.

Recommendations

Refactor "epochs" in VotingEscrow.sol to something like "update_count".

[NC-02] Comment typo in GaugeController.vote_for_gauge_weights() (Lines 215-220)

Notice below that the first comment indicating a return value is not in the correct position:

        (
            ,
            /*int128 bias*/
            int128 slope_, /*uint256 ts*/

        ) = ve.getLastUserPoint(msg.sender);

Recommendations

        (
+           /*int128 bias*/,
-           /*int128 bias*/
            int128 slope_, /*uint256 ts*/

        ) = ve.getLastUserPoint(msg.sender);

[NC-03] Unnecessary variable declaration in GaugeController._change_gauge_weight() (Lines 196-197)

Notice below that new_sum is only used to set points_sum[next_time].bias.

    function _change_gauge_weight(address _gauge, uint256 _weight) internal {

<details> <Summary> (Show/Hide extra code) </Summary>

        uint256 old_gauge_weight = _get_weight(_gauge);
        uint256 old_sum = _get_sum();
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;

        points_weight[_gauge][next_time].bias = _weight;
        time_weight[_gauge] = next_time;

</details>

        uint256 new_sum = old_sum + _weight - old_gauge_weight;
        points_sum[next_time].bias = new_sum;
        time_sum = next_time;
    }

Recommendations

-       uint256 new_sum = old_sum + _weight - old_gauge_weight;
-       points_sum[next_time].bias = new_sum;
+       points_sum[next_time].bias = old_sum + _weight - old_gauge_weight;
        time_sum = next_time;
    }

[NC-04] GaugeController._change_gauge_weight() does not need to change the value of time_sum (Line 198)

_change_gauge_weight() sets next_time equal to the end of the week, then sets time_sum equal to next_time. This is unnecessary because _change_gauge_weight() calls _get_sum() earlier in the function, which sets time_sum equal to the end of the week. See below inline comments in _get_sum():

    function _get_sum() internal returns (uint256) {
        uint256 t = time_sum; //t is a timestamp marking the end of an Epoch/week
        Point memory pt = points_sum[t];
        for (uint256 i; i < 500; ++i) {
            if (t > block.timestamp) break;
            t += WEEK; //t is incremented by 1 week until it equals the end of the current week, then the for loop breaks
            uint256 d_bias = pt.slope * WEEK;
            if (pt.bias > d_bias) {
                pt.bias -= d_bias;
                uint256 d_slope = changes_sum[t];
                pt.slope -= d_slope;
            } else {
                pt.bias = 0;
                pt.slope = 0;
            }
            points_sum[t] = pt;
            if (t > block.timestamp) time_sum = t; //time_sum is always set to t, equal to the end of the current week
        }
        return pt.bias;
    }

Recommendations

-       time_sum = next_time;
    }

[NC-05] NatSpec/Comments for VotingEscrow._checkpoint() params state "null" instead of "default values" (Lines 113-114)

Solidity does not have a null type; the comments should state something like "default values" for accuracy. (Notice that the params referred to are structs, and the global updating function checkpoint() calls _checkpoint with default-valued structs passed as arguments.)

    /// @notice Public function to trigger global checkpoint
    function checkpoint() external {
        LockedBalance memory empty;
        _checkpoint(address(0), empty, empty);
    }

    /// @notice Records a checkpoint of both individual and global slope
    /// @param _addr User address, or address(0) for only global
    /// @param _oldLocked Old amount that user had locked, or null for global
    /// @param _newLocked new amount that user has locked, or null for global
    function _checkpoint(
        address _addr,
        LockedBalance memory _oldLocked,
        LockedBalance memory _newLocked  
    ) internal {

Recommendations

Change "null" to "default valued LockedBalance"

[NC-06] NatSpec/Comments for VotingEscrow._checkpoint() can be revised for clarity (Line 111)

The @notice comment can change from "Records a checkpoint of both individual and global slope" to "Records a checkpoint of global slope, or both individual and global slope", since no individual checkpoint will be recorded if _checkpoint is called from checkpoint().

Recommendations

Apply stated change

[NC-07] VotingEscrow.sol refers to IVotingEscrow for the NatSpec of certain functions, but IVotingEscrow does not exist in the repo

Recommendations

Fill in missing NatSpec; alternatively, add IVotingEscrow to the repo and use @inheritdoc

[NC-08] VotingEscrow.sol functions createLock, increaseAmount, and delegate have unnecessary nonReentrant modifiers (Lines 268, 288, 356)

These functions do not contain any external calls, so reentrancy is not an issue.

Recommendations

Remove the nonReentrant modifiers from the stated functions.

[NC-09] State variables in GaugeController.sol and VotingEscrow.sol are undocumented

Recommendations

In-line comments can be added to the state variables in GaugeController.sol and VotingEscrow.sol for reader clarity

[NC-10] LendingLedger.sol imports VotingEscrow, but it's unused

Recommendations

-import {VotingEscrow} from "./VotingEscrow.sol";

[NC-11] LendingLedger.setRewards can list the onlyGovernance modifier first (Line 192)

Modifiers are executed left-to-right; consider listing the onlyGovernance modifier first since it's the most important one.

Recommendations

    function setRewards(
        uint256 _fromEpoch,
        uint256 _toEpoch,
        uint248 _amountPerEpoch
-   ) external is_valid_epoch(_fromEpoch) is_valid_epoch(_toEpoch) onlyGovernance {
+   ) external onlyGovernance is_valid_epoch(_fromEpoch) is_valid_epoch(_toEpoch) {

[NC-12] LendingLedger.claim() silently fails if the user has already claimed their rewards for the week

Silent failure also occurs if the _claimUpToTimestamp argument is less than the _claimFromTimestamp argument (as long as both are valid epochs).

Recommendations

-       if (claimEnd >= claimStart) { // If the check here fails, the function execution silently ends
+       require(claimEnd >= claimStart, "Invalid claim") // NOTE: This should be a custom error for gas savings
            // This ensures that we only set userClaimedEpoch when a claim actually happened
            for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
                uint256 userBalance = lendingMarketBalances[_market][lender][i];
                uint256 marketBalance = lendingMarketTotalBalance[_market][i];
                RewardInformation memory ri = rewardInformation[i];
                require(ri.set, "Reward not set yet"); // Can only claim for epochs where rewards are set, even if it is set to 0
                uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
                cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
            }
            userClaimedEpoch[_market][lender] = claimEnd + WEEK;
-       }