code-423n4 / 2023-08-verwa-findings

8 stars 7 forks source link

Fixed setRewards can make the protocol economically unviable. #324

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

This finding is based on the assumption that the goal of the protocol is to have a constant inflow of new users who will lock CANTO in order to receive a reward in CANTO.

The current implementation will make the protocol economically nonviable through the direct link between increased users and decreased rewards, thereby lowering rewards for existing users and completely halting the inflow of new users.

From documentation:

In other words, users can stake CANTO, receive veCANTO to vote and claim CANTO through LendingLedger as interest.

NOTE: The influx of staked CANTO is DYNAMIC, it will only increase if ROI received from the claims is economically viable (i.e. sufficiently close to other similar products).

The setRewards function sets the amount of CANTO rewards per epoch and this cannot be changed. The require(!ri.set, "Rewards already set") ensures that not even Governance can change the rewards for a set of epochs once they have been set.

    function setRewards(
        uint256 _fromEpoch,
        uint256 _toEpoch,
        uint248 _amountPerEpoch
    ) external is_valid_epoch(_fromEpoch) is_valid_epoch(_toEpoch) onlyGovernance {
        for (uint256 i = _fromEpoch; i <= _toEpoch; i += WEEK) {
            RewardInformation storage ri = rewardInformation[i];
            require(!ri.set, "Rewards already set");
            ri.set = true;
            ri.amount = _amountPerEpoch;
        }
    }

NOTE: The amount of CANTO paid per epoch is FIXED.

A dynamic input and fixed output means that the amount of CANTO received by users of the protocol will decrease for each additional user that is staking CANTO and will decrease until near 0 or until it becomes economically nonviable to do and there will be no more new users until old users can unstake after the 5 year period and ROI rises once more.

Proof of Concept

Protocol calls setRewards to set the rewards for the next year and transfers CANTO to the LendingLedger contract. The rewards have been set so that an estimated amount of users k will be able to claim an amount of CANTO c each epoch. This will represent an ROI sufficiently close to similar products to be of economic interest.

NOTE. Setting setRewards to one epoch would avoid this problem, but this requires constant manual updates which are risky in itself and the function is clearly aimed at setting the rewards for a longer duration through the for loop.

veRWA launches and is a massive success. After 3 months the amount of users is double what was assumed 2*k and the rewards c per epoch per user have halved. This bring its below an ROI sufficient to be of economic interest and for the remainder of the year (9 months), there are no new users.

Tools Used

Manual Review

Recommendations

Options: 1) Adapt setRewards so that rewards can be changed for future epochs even though they have already been set. 2) Enable a link between the dynamic input and rewards, so that the rewards will slightly adjusted for next epoch so that the ROI will always remain economically viable and the protocol will keep attracting new users.

Assessed type

Other

141345 commented 1 year ago

effects and mechanism about inflation of rewards

more suitable for analysis report

c4-sponsor commented 1 year ago

OpenCoreCH marked the issue as sponsor acknowledged

OpenCoreCH commented 1 year ago

Can be solved by setting the rewards regularly (e.g., every month) and letting governance decide on how many tokens to allocate for the next few epochs (for instance, based on users, user growth, or other metrics)

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory

alcueca commented 1 year ago

The warden is showing that the current implementation is economically unviable, not that the design is economically unviable. Therefore this is Medium and not analysis.

JeffCX commented 1 year ago

Can be solved by setting the rewards regularly (e.g., every month) and letting governance decide on how many tokens to allocate for the next few epochs (for instance, based on users, user growth, or other metrics)

based on this comments, seems like the current implementation is a design choice, but a vulnerabilty, so I politely suggest the severity level is QA

alcueca commented 1 year ago

I didn't fully grasp the mitigation proposed by the sponsor. While it is true that rewards for already set epochs can't be changed, there is nothing forcing governance to set rewards for very long periods of time, and there is nothing either forcing governance to always set the same rewards.

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

alcueca marked the issue as grade-a

14si2o commented 1 year ago

Hello,

I would like to offer a counter argument concerning the impact of this finding.

As @alcueca mentions, there is nothing forcing the sponsor to set the duration of the rewards and if the duration is set very low (1/2 epochs), it is perfectly feasible to adapt the amount of tokens to be issued on a short basis and avoid any problems.

However, it is important to note that the function is clearly designed to set the rewards for multiple epochs. With no indication to how many epochs will used as duration.

Therefore, the critical question that determines if this finding is medium or QA is:

If the sponsor was acutely aware that setting a very short duration in setRewards is pivotal to be able to react to dynamic demand, then this finding is merely a QA.

If the sponsor was however not aware, if there were no discussions on the maximum duration of setRewards to avoid dwindling amounts of new users, then I believe it would be correct set this finding as Medium.

@OpenCoreCH

Could you be so kind as to weigh in?

Thank you, Flint

alcueca commented 1 year ago

Upon further consideration, I realize that most rewards systems distribute a fixed amount of assets over a fixed amount of time, and this is no different.

alcueca commented 1 year ago

Maintaining the QA grade because the recommendation to allow changing the rewards for already set but future epochs might be of interest.