veRWA - cducrest's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L129-L143 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L55-L78

Vulnerability details

Impact

Users can add liquidity to markets right before the end of a week and withdraw it right after the end of the week to be counted towards having provided liquidity for the week.

As a result, lenders can provide liquidity for only 1 block and received as much reward as honest lenders who locked their tokens for extensive period of times. If a lender is able to manipulate transaction ordering, they can be sure their transaction providing liquidity is the last of the week-closing block and their transaction removing liquidity the first of the week-opening block. As such they can be sure their liquidity is never used by the market.

In any case, the odds of their liquidity being useful to the market are fairly slim.

Attackers can borrow a lot of cNOTEs for just a couple of blocks and provide liquidity with them to reap rewards. Honest users will receive less reward due to attackers receiving some.

Proof of Concept

The function sync_ledger() to be called by lending markets on deposits / withdrawals calls _checkpoint_lender() and increase / decrease the lendingMarketBalances for current epoch and market:

    /// @notice Function that is called by the lending market on cNOTE deposits / withdrawals
    /// @param _lender The address of the lender
    /// @param _delta The amount of cNote deposited (positive) or withdrawn (negative)
    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);
        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
        require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
        lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);

        ...
    }

The function _checkpoint_lender() sets userClaimedEpoch to current epoch if the user never deposited or does not update it if user already deposited:

    function _checkpoint_lender(
        address _market,
        address _lender,
        uint256 _forwardTimestampLimit
    ) private {
        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;

        uint256 lastUserUpdateEpoch = lendingMarketBalancesEpoch[_market][_lender];
        uint256 updateUntilEpoch = Math.min(currEpoch, _forwardTimestampLimit);
        if (lastUserUpdateEpoch == 0) {
            // Store epoch of first deposit
            userClaimedEpoch[_market][_lender] = currEpoch;
            lendingMarketBalancesEpoch[_market][_lender] = currEpoch;
        } else if (lastUserUpdateEpoch < currEpoch) {
            // Fill in potential gaps in the user balances history
            uint256 lastUserBalance = lendingMarketBalances[_market][_lender][lastUserUpdateEpoch];
            for (uint256 i = lastUserUpdateEpoch; i <= updateUntilEpoch; i += WEEK) {  // @audit-ok first iteration useless
                lendingMarketBalances[_market][_lender][i] = lastUserBalance;
            }
            if (updateUntilEpoch > lastUserUpdateEpoch) {
                lendingMarketBalancesEpoch[_market][_lender] = updateUntilEpoch;
            }
        }
    }

When claiming, user is entitled to reward starting from (and including) userClaimedEpoch[_market][lender]:

    function claim(
        address _market,
        uint256 _claimFromTimestamp,
        uint256 _claimUpToTimestamp
    ) external is_valid_epoch(_claimFromTimestamp) is_valid_epoch(_claimUpToTimestamp) {
        address lender = msg.sender;
        uint256 userLastClaimed = userClaimedEpoch[_market][lender];
        require(userLastClaimed > 0, "No deposits for this user");
        _checkpoint_lender(_market, lender, _claimUpToTimestamp);
        _checkpoint_market(_market, _claimUpToTimestamp);
        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
        uint256 claimStart = Math.max(userLastClaimed, _claimFromTimestamp);
        uint256 claimEnd = Math.min(currEpoch - WEEK, _claimUpToTimestamp);
        uint256 cantoToSend;
        if (claimEnd >= claimStart) {
            // 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;
        }
        if (cantoToSend > 0) {
            (bool success, ) = msg.sender.call{value: cantoToSend}("");
            require(success, "Failed to send CANTO");
        }
    }

Tools Used

Manual review, testing.

Here's a test that can be added to the end of LendingLedger.t.sol:

    function testAddLiquidityForShortTime() public {
        whiteListMarket();

        vm.prank(goverance);
        ledger.setRewards(fromEpoch, toEpoch, amountPerEpoch);  // from week 5 to week 10

        // vm.warp(WEEK * 5);

        vm.warp(WEEK * 6 - 1);
        int256 delta = 1.1 ether;
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, delta);

        vm.warp(WEEK * 6);
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, delta);

        // airdrop ledger enough token balance for user to claim
        payable(ledger).transfer(1000 ether);

        vm.warp(WEEK * 20);

        uint256 balanceBefore = address(lender).balance;
        vm.prank(lender);
        ledger.claim(lendingMarket, fromEpoch, fromEpoch);
        uint256 balanceAfter = address(lender).balance;
        assertTrue(balanceAfter - balanceBefore == 1 ether);

        uint256 claimedEpoch = ledger.userClaimedEpoch(lendingMarket, lender);
        assertTrue(claimedEpoch - fromEpoch == WEEK);
    }

Recommended Mitigation Steps

Only count participation for future epochs, i.e. make sync_ledger() update balance in lendingMarketBalances and lendingMarketTotalBalance for currentEpoch + 1. Make _checkpoint_lender() set the initial values for userClaimedEpoch to currentEpoch + 1.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The algorithm to fill gauge weights in _get_weight() does not do anything in most common situations where governance did not intervene to overwrite gauge weights.

This is a problem to fill historical data for missed updates for any gauges during a week. The value of 0 will be returned when querying about the gauge in vote_for_gauge_weights() and the wrong vote values may be computed for any gauge missing a week of updates. The value computed in gauge_relative_weight_write() for gauges that missed an update will be incorrect for historical values.

Proof of Concept

The algorithm in _get_weight() starts by checking if time_weight[_gauge_addr] > 0 and does nothing if that is not the case:

    function _get_weight(address _gauge_addr) private returns (uint256) {
        uint256 t = time_weight[_gauge_addr];
        if (t > 0) {
            ...
        } else {
            return 0;
        }
    }

This value starts at 0 when a gauge is added:

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

The value is only updated by the _get_weight() function (when it already is non-zero), or in _change_gauge_weight()

    function _change_gauge_weight(address _gauge, uint256 _weight) internal {
        ...
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;
        ...
        time_weight[_gauge] = next_time;
        ...
    }

The _change_gauge_weight() function is only called in functions protected by onlyGovernance used to overwrite a gauge weight or remove a gauge:

    /// @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);
    }

    ...

    /// @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 {
        _change_gauge_weight(_gauge, _weight);
    }

Overall time_weight[_gauge_addr] is never set to a non-zero value under normal circumstances.

Tools Used

Manual review, testing

Recommended Mitigation Steps

When adding a gauge set the value of time_weight[_gauge_addr] to current epoch.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users cannot withdraw a delegated lock, they have to delegate the lock back to themselves. This requires the expiry date of their own lock to be greater than the lock of the delegatee. Since user delegated their locks to delegatee in the first place, the expiry of delegatee's lock must be higher than their locks.

As a result, user needs to increase their lock's expiry date by calling increaseAmount(tinyValue), which will revert if delegatee's lock is expired.

If delegatee's lock is expired, users will not be able to increase their lock's end duration to delegate to themselves and will not be able to withdraw their lock at any point in time.

Proof of Concept

Withdrawing from VotingEscrow requires the lock to be delegated to lock owner:

    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");
        ...
    }

Owner thus needs to delegate to himself before withdrawing if they delegated to someone else before. Doing so requires the lock end time of the user to be greater than the lock end time of the delegate:

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            ...
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];  // @audit-info we remove power from old delegatee
            toLocked = locked_;  // @audit-info we add power to msg.sender
        } else {
            ...
        }
        ...
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        ...
    }

The only way to increase your lock end time past the delegatee lock end time is by calling increaseAmount(value), which will revert if delegatee's lock expired:

    function increaseAmount(uint256 _value) external payable nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        address delegatee = locked_.delegatee;
        ...
        if (delegatee == msg.sender) {
            ...
        } else {
            ...
            locked_ = locked[delegatee];
            require(locked_.amount > 0, "Delegatee has no lock");
            require(locked_.end > block.timestamp, "Delegatee lock expired");  // @audit cannot increaseAmount if delegatee expired -> cannot change delegate
            ...
        }
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Do not require that the former delegatee's lock be not expired to increase amount.

Alternatively, allow withdrawing delegate lock or do not impose a condition on expiry time for undelegating a lock.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users cannot withdraw a delegated lock, they have to delegate the lock back to themselves. This is impossible if their lock is expired.

Users cannot withdraw delegated locks when expired.

Proof of Concept

Withdrawing from VotingEscrow requires the lock to be delegated to lock owner:

    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");
        ...
    }

Owner thus needs to delegate to himself before withdrawing if they delegated to someone else. Doing so will revert if the user lock is expired

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        address delegatee = locked_.delegatee;
        ...
        if (delegatee == msg.sender) {
            ...
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        ...
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Allow undelegating a lock when expired.

Alternatively, allow withdrawing delegated lock.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users cannot withdraw a delegated lock, they have to delegate the lock back to themselves. This requires the expiry date of their own lock to be greater than the lock of the delegatee. Since user delegated their locks to delegatee in the first place, the expiry of delegatee's lock must be higher than their locks.

As a result, user needs to increase their lock's expiry date by calling increaseAmount(tinyValue), delegate to themselves, wait 5 years, and call withdraw().

In any case, the owner of a delegated lock will have to wait 5 years before withdrawing.

I believe this should deter anyone from delegating a lock and participating in this protocol.

Proof of Concept

Withdrawing from VotingEscrow requires the lock to be delegated to lock owner:

    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");
        ...
    }

Owner thus needs to delegate to himself before withdrawing if they delegated to someone else before. Doing so requires the lock end time of the user to be greater than the lock end time of the delegate:

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            ...
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];  // @audit-info we remove power from old delegatee
            toLocked = locked_;  // @audit-info we add power to msg.sender
        } else {
            ...
        }
        ...
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        ...
    }

The only way to increase your lock end time past the delegatee lock end time is by calling increaseAmount(value), which will reset your lock duration to 5 years:

    function increaseAmount(uint256 _value) external payable nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        require(locked_.end > block.timestamp, "Lock expired");
        // Update lock
        address delegatee = locked_.delegatee;
        ...
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount += int128(int256(_value));
        newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);
        if (delegatee == msg.sender) {
            ...
        } else {
            // Delegated lock, update sender's lock first
            locked[msg.sender] = newLocked;
            ...
        }
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Allow undelegating a lock without changing the lock expiry time. I.e. allow users to set themselves as delegatee of their lock with no requirement about the user's lock end time being greater than the former delegatee's lock end time.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The documentation of VotingEscrow.delegate() states to see IVotingEscrow. As stated on the discord channel for the contest, the reference implementation can be found here. The reference implementation states:

/// @dev Delegator inherits updates of delegatee lock

While in the code, the delegatee lock's end time can be updated via increaseAmount() without updating the delegator's end.

Users may be mislead as to what their lock's end time is if they read the pointed documentation.

Proof of Concept

IVotingEscrow from fiat-dao:

    /// @notice Delegate voting power to another address
    /// @param _addr user to which voting power is delegated
    /// @dev Can only undelegate to longer lock duration
    /// @dev Delegator inherits updates of delegatee lock
    function delegate(address _addr) external;

While increaseAmount() does not update the delegator's lock end time:

    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");
        // Update lock
        address delegatee = locked_.delegatee;
        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);
        if (delegatee == msg.sender) {
            // Undelegated lock
            action = LockAction.INCREASE_AMOUNT_AND_DELEGATION;
            newLocked.delegated += int128(int256(_value));
            locked[msg.sender] = newLocked;
            _checkpoint(msg.sender, locked_, newLocked);
        } else {
            // Delegated lock, update sender's lock first
            locked[msg.sender] = newLocked;
            _checkpoint(msg.sender, locked_, newLocked);
            // Then, update delegatee's lock and voting power (checkpoint)
            locked_ = locked[delegatee];
            require(locked_.amount > 0, "Delegatee has no lock");
            require(locked_.end > block.timestamp, "Delegatee lock expired");
            newLocked = _copyLock(locked_);
            newLocked.delegated += int128(int256(_value));
            locked[delegatee] = newLocked;
            _checkpoint(delegatee, locked_, newLocked);
            emit Deposit(delegatee, _value, newLocked.end, LockAction.DELEGATE, block.timestamp);
        }
        emit Deposit(msg.sender, _value, unlockTime, action, block.timestamp);
    }

One could argue that the delegatee's lock end time update impacts the delegator's lock as the delegator's lock cannot be withdrawn unless self-delegated and self-delegating requires the user's lock end time to be greater than the delegatee's lock end time. However, this is not sufficient in the situation where the the delegatee itself delegates to a third party:

• A, B, and C own locks with end time x
• A delegates to B
• B delegates to C
• C updates their lock's end time via calling increaseAmount() to x + y
• B cannot withdraw their locks before time x + y because they need to self-delegate and that requires B's lock end time >= C lock's end time
• A can still self-delegate since A's lock end time >= B's lock's end time
• A can withdraw their locks at end time x < x + y.

Tools Used

Manual review

Recommended Mitigation Steps

Evaluate whether the withdraw condition is what you want it to be, especially in case of a chain of delegated locks. Update delegator's lock end time when delegatee's lock is updated.

Alternatively, update documentation to make behaviour explicit.

Assessed type

Context