veRWA - deth's results

Lines of code

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

Vulnerability details

Impact

The likelihood of this happening is very small, but in my opinion it should be addressed, because it can have an effect on some users. Currently claim reverts if rewardInformation isn't set for the specified epoch. If the governance doesn't set a reward for a specific epoch for quite some time, this may force some users to forfeit their rewards for that epoch, so they can continue claiming rewards for the following epochs.

Imagine the following scenario: Epoch 1 begins and the governance sets the rewards for the epoch. Epoch 2 begins, but the governance can't agree on what reward it should have. This can be because of many different reasons, the epoch is special in some way and the rewards must be higher/lower respectively, someone is griefing the vote, etc... Epoch 3 begins and the rewards are set, but Epoch 2 still doesn't have any rewards set. Time passes and Epoch 2 still doesn't have any rewards set. Users can claim the rewards for Epoch 1 no problem, but if they want to claim the rewards for Epoch 3, they will forfeit their rewards for Epoch 2, as their userClaimedEpoch will point to Epoch 3 and users can't claim epochs in the past.

Proof of Concept

Paste this test in LendingLedger.t.sol and run forge test --mt testCannotClaimEpochWithNoRewardsForceUsersToForfeitRewardsInOrderToClaimNextEpochs-vvvv.

function testCannotClaimEpochWithNoRewardsForceUsersToForfeitRewardsInOrderToClaimNextEpochs() public {
        uint248 amountPerEpoch = 1 ether;

        // Set up for both the epochs.
        // nORewardsEpoch = The epoch where governance still hasn't decided what the rewards should be
        // rewardsEpoch = The next epoch where the governance have decided what the rewards should be
        uint256 rewardsEpoch1 = WEEK * 1;
        uint256 noRewardsEpoch = WEEK * 2;
        uint256 rewardsEpoch2 = WEEK * 3;

        // Setup the lending market
        address lendingMarket = vm.addr(5201314);
        vm.prank(goverance);
        ledger.whiteListLendingMarket(lendingMarket, true);

        // An epoch passes and governance setRewards for the first epoch (rewardsEpoch1)
        vm.warp(WEEK * 1);
        vm.prank(goverance);
        ledger.setRewards(rewardsEpoch1, rewardsEpoch1, amountPerEpoch);

        // Another epoch passes, but governance still hasn't decided on the rewards for the second epoch (noRewardsEpoch)
        // This may happen if this is an important epoch for example, where the rewards must be higher/lower 
        // and the governance can't decide on an exact rewards amount.
        vm.warp(WEEK * 2);

        // A lender deposits funds in a lending market during the second epoch (noRewardsEpoch)
        address lender = users[1];
        int256 delta = 1.1 ether;
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, delta);
        payable(ledger).transfer(1000 ether);

        // Another epoch passes. Governance still haven't set a reward for the second epoch (noRewardsEpoch), but now
        // the third epoch (rewardsEpoch2) needs it's rewards set.
        // The governance sets the rewards for the third epoch.
        vm.warp(WEEK * 3);
        vm.prank(goverance);
        ledger.setRewards(rewardsEpoch2, rewardsEpoch2, amountPerEpoch);

        // If the user now attempts to claim his rewards from the second epoch (noRewardsEpoch) up to the last epoch (rewardsEpoch2),
        // the tx will revert as there is still no rewards set for the second epoch.
        vm.startPrank(lender);
        vm.expectRevert("Reward not set yet");
        ledger.claim(lendingMarket, noRewardsEpoch, rewardsEpoch2);
        vm.stopPrank();
        
        // The second epoch (noRewardsEpoch) might not have it's rewards set for a long time, in a way forcing
        // users to skip that certain epoch and forfeiting their rewards, if they want to claim the rewards from following epochs.
    }

Tools Used

Recommended Mitigation Steps

I propose the following, to mitigate this issue:

Instead of reverting, add the epoch, that doesn't have rewards set yet, in a mapping. This mapping will hold the address of the user => epoch that didn't have rewards => a boolean to indiciate if the reward has been set (mapping(address => mapping(uin256 => bool)) unclaimedEpochsForLender) and create another function that lenders can call, in which they can claim rewards for previously skipped and unclaimed epochs claimPreviouslyUnclaimedEpoch.

The following is a rough example of what the logic can look like.

Mapping which holds what user for what epoch, couldn't claim the rewards weren't set

mapping(address => mapping(uint256 => bool)) unclaimedEpochsForLender;

Function that will claim an epoch, for a specific market and a specific lender that hadn't had it's rewards set in the past and the user couldn't claim his rewards.

    function claimPreviouslyUnclaimedEpoch(
        address _market, 
        uint256 _epoch
    ) external is_valid_epoch(_epoch) {
        address lender = msg.sender;
        uint256 cantoToSend = 0;

        RewardInformation memory ri = rewardInformation[_epoch];
        require(ri.set, "Reward not set.");
        require(unclaimedEpochsForLender[lender][_market][_epoch], "Epoch already claimed");

        uint256 userBalance = lendingMarketBalances[_market][lender][_epoch];
        uint256 marketBalance = lendingMarketTotalBalance[_market][_epoch];
        uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, _epoch); // Normalized to 1e18
        cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance);
        unclaimedEpochsForLender[lender][_market][_epoch] = false;

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

Modified version of claim.

    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];
                // New logic goes here.
                if (!ri.set) {
                    unclaimedEpochsForLender[lender][_market][i] = true;
                    continue;
                } else {
                    unclaimedEpochsForLender[lender][_market][i] = false;
                }
                uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
                //@audit Possible precision loss
                cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
            }
            userClaimedEpoch[_market][lender] = claimEnd + WEEK;
        }
        if (cantoToSend > 0) {
            //@audit Can we break this?
            (bool success, ) = msg.sender.call{value: cantoToSend}("");
            require(success, "Failed to send CANTO");
        }
    }

POC to show the flow of the logic.

Paste this test in LendingLedger.t.sol and run forge test --mt testClaimPreviouslyUnclaimedEpoch -vvvv.

     function testClaimPreviouslyUnclaimedEpoch() public {
        uint248 amountPerEpoch = 1 ether;

        // Set up for both the epochs.
        // nORewardsEpoch = The epoch where governance still hasn't decided what the rewards should be
        // rewardsEpoch = The next epoch where the governance have decided what the rewards should be
        uint256 rewardsEpoch1 = WEEK * 1;
        uint256 noRewardsEpoch = WEEK * 2;
        uint256 rewardsEpoch2 = WEEK * 3;

        // Setup the lending market
        address lendingMarket = vm.addr(5201314);
        vm.prank(goverance);
        ledger.whiteListLendingMarket(lendingMarket, true);

        // An epoch passes and governance setRewards for the first epoch (rewardsEpoch1)
        vm.warp(WEEK * 1);
        vm.prank(goverance);
        ledger.setRewards(rewardsEpoch1, rewardsEpoch1, amountPerEpoch);

        // Another epoch passes, but governance still hasn't decided on the rewards for the second epoch (noRewardsEpoch)
        // This may happen if this is an important epoch for example, where the rewards must be higher/lower 
        // and the governance can't decide on an exact rewards amount.
        vm.warp(WEEK * 2);

        // A lender deposits funds in a lending market during the second epoch (noRewardsEpoch)
        address lender = users[1];
        int256 delta = 1.1 ether;
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, delta);
        payable(ledger).transfer(1000 ether);

        // Another epoch passes. Governance still haven't set a reward for the second epoch (noRewardsEpoch), but now
        // the third epoch (rewardsEpoch2) needs it's rewards set.
        // The governance sets the rewards for the third epoch.
        vm.warp(WEEK * 3);
        vm.prank(goverance);
        ledger.setRewards(rewardsEpoch2, rewardsEpoch2, amountPerEpoch);

        // If the user now attempts to claim his rewards from the second epoch (noRewardsEpoch) up to the last epoch (rewardsEpoch2),
        // the tx will revert as there is still no rewards set for the second epoch.
        vm.prank(lender);
        ledger.claim(lendingMarket, noRewardsEpoch, rewardsEpoch2);

        // Time passes and governance sets the rewards for the second epoch (noRewardsEpoch).
        vm.warp(WEEK * 4);
        vm.prank(goverance);
        ledger.setRewards(noRewardsEpoch, noRewardsEpoch, amountPerEpoch);

        // The lender claims his previously unclaimed epoch.
        uint256 balanceBeforePreviousClaim = address(lender).balance;
        vm.prank(lender);
        ledger.claimPreviouslyUnclaimedEpoch(lendingMarket, noRewardsEpoch);
        uint256 balanceAfterPreviousClaim = address(lender).balance;
        assertGt(balanceAfterPreviousClaim, balanceBeforePreviousClaim);
    }

Assessed type

Other