c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L1 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L164

Vulnerability details

Impact

Function addMarket is responsible for adding new markets. It is protected by internal _ensureMaxLoops which checks if (defined by _setMaxLoopsLimit) limit of loops iterations is not exceeded. Since _ensureMaxLoops checks the maximum number of loops iterations with addMarkets.length - it basically checks maximum number of markets in the protocol. Since, in multiple of places - we're iterating over allMarkets.length - the function _ensureMaxLoops checks if the defined maximum number of iterations is not exceeded (this protects from potential DoS and potential out of gas):

File: contracts/MaxLoopsLimitHelper.sol


    /**
     * @notice Set the limit for the loops can iterate to avoid the DOS
     * @param limit Limit for the max loops can execute at a time
     */
    function _setMaxLoopsLimit(uint256 limit) internal {

The maximum number of loops is, however, set in the initialize function only:

File: contracts/Tokens/Prime/Prime.sol

 _setMaxLoopsLimit(_loopsLimit);

thus, it cannot be changed later.

The check - if we haven't exceeded the _setMaxLoopsLimit is performed in function _ensureMaxLoops(). This function is being called every time we add new market in addMarket(). It won't allow us to add more markets than it was defined by _setMaxLoopsLimit.

However, across the whole contract - there's no a single function which allows to remove the previously added market.

Those constitute multiple of issues related with protocol's design:

when some malicious market will be added - it's not possible to remove it
when some market will be added by mistake - it won't be possible to remove it - and it will count to the _ensureMaxLoops limit
right now Venus Protocol doesn't support underlying tokens with more than 18 decimals, if market with underlying token with more than 18 decimals will be added by mistake - it should be considered as invalid - however, it won't be possible to remove it
protocol will be usable as long as added markets are valid, when, e.g. some invalid markets will be added - and number of added market will exceed the limit enforced by _ensureMaxLoops - noone will be able to add any new market

This, according to Code4rena Severity Categorization is defined as:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Hypothetical attack path - invalid market is being added by mistake and it cannot be removed Availability of the protocol is impacted - added market counts to the limit enforced by _ensureMaxLoops. If _ensureMaxLoops enforces max N markets available in the system and some invalid market is being added by mistake - then the limit is N-1 and cannot be changed. In some hypothetical scenario, when _ensureMaxLoops allows only one market and that market is added by mistake - protocol is fully non-usable. If _ensureMaxLoops allows only 2 markets to be added - and one market is added by mistake, than usability of protocol is reduced to 50% (because only one more valid market can be added).

The additional issue - in our opinion - much more impactful - is related to the current gas cost. Since multiple parts of the protocol iterates over allMarkets, e.g.:

_accrueInterestAndUpdateScore
_initializeMarkets
_burn
getPendingInterests
updateScores

the _setMaxLoopsLimit should be set based on the current maximum gas cost. If we set this value to be too big - then the protocol would allow to add too many markets - and looping through those markets would cause out of gas exception (protocol won't be usable). This implies, that the parameter for _setMaxLoopsLimit has to be thoughtfully set. Especially, it can be set one time only (in initialize). However, current implementation does not take into a consideration a scenario, when maximum gas cost drastically decreases. When the maximum gas cost drastically decreases, the previously set _setMaxLoopsLimit will always be too big and every loop will revert due to out of gas. The only solution to fix that state would be to remove some markets from allMarkets - however, protocol does not implement any function which allows to do that. This is another hypothetical state which impacts the protocol's availability (which has been described as Medium by Code4rena Severity Categorization).

Another critical scenario is - setting _setMaxLoopsLimit parameter to too big value. If _setMaxLoopsLimit enforces too big limit - we'll be able to create a lot of new markets. If there will be too many markets - we won't be able to iterate over them, due to out of gas exception.

Proof of Concept

Line 306 of Prime.sol does not allow to add more markets than it was set by _setMaxLoopsLimit:

File: contracts/Tokens/Prime/Prime.sol

        _ensureMaxLoops(allMarkets.length);

Moreover, across the whole contract, there's no function which allows to remove the market. This implies, that whenever some market will be added (either by mistake, or the market will become invalid at some point in the future) - then it's not possible to remove it from the protocol.

In the Impact section, multiple of scenarios had been described, The PoC will present the last scenario - setting incorrect _setMaxLoopsLimit, which will lead to protocol been fully non-usable.

_setMaxLoopsLimit is being set (by mistake, or due to improper calculation of gas cost) to very high value
function addMarkets adds multiple of new markets
Since _setMaxLoopsLimit set very high limit, function _ensureMaxLoops(allMarkets.length) allows to add many new markets
Now, function _burn is being called, it iterates over allMarkets, but since there are too many markets, it reverts with out of gas error.
Protocol owner cannot reduce the number of added market - thus burning prime token is not possible (function will always revert) - protocol becomes non-usable.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement additional function which allows to remove previously added market. Moreover, implement a reinitialize function, which would allow to adjust the maximum number of markets in the future (in other words, allow to call _setMaxLoopsLimit with different value to adjust the maximum number of markets).

Assessed type

Loop

c4-pre-sort commented 1 year ago

0xRobocop marked the issue as duplicate of #421

c4-judge commented 11 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)