search

code-423n4 / 2023-05-venus-findings

2 stars 1 forks source link

[M1] maxLoopsLimit cannot be decreased #549

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/MaxLoopsLimitHelper.sol#L26

Vulnerability details

Impact

If the owner set maxLoopLimit too high then you need to upgrade the contract and your code to avoid possible DOS.

Proof of Concept

The variable maxLoopLimit is touched by admins of contracts that inherits them.

I have noticed that in your design you do not allow to decrease that variable 

function _setMaxLoopsLimit(uint256 limit) internal {
    require(limit > maxLoopsLimit, "Comptroller: Invalid maxLoopsLimit");

To me this is an error because generally when you set a max variable you compare with your previous version.
However, in this case is not recommended.

Sample Poc

The owner increase a little bit maxLoops.

Then she noticed that the value is too high because it certainly causes DOS .

You will need to upgrade your contract, even you need to upgrade you code as well. The reason is that initialize() will fail in the new upgraded contract.

Recommended

I suggest you that MaxLoopLimitHelper can be set between a range of reasonable parameters by the owner like MINMAXLOOPS and MAXMAXLOOPS and avoid zero of course.

Assessed type

Other

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by 0xean

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by 0xean

c4-judge commented 1 year ago

0xean marked the issue as duplicate of #386

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

0xean marked the issue as grade-c