Open code423n4 opened 1 year ago
getAllMarkets
is a view function and can't be DOS'ed unless being used in a state changing function which doesn't exit inside the codebase.
Invalid.
0xSorryNotSorry marked the issue as low quality report
getAllMarkets
is view, but allMarkets
gets used in state-changing function, and the documentation should highlight that governance must be careful with how many markets it adds, because they add to the gas cost of certain functions, and can't be removed. Being able to remove markets would be even better.
alcueca changed the severity to QA (Quality Assurance)
alcueca marked the issue as grade-b
Lines of code
https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L772-L798 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L1060-L1062
Vulnerability details
Impact
When the smart contracts start to be used, the variable in storage
allMarkets
will start to be filled withtokens
, as there is no mechanism to eliminate elements, this will cause thegetAllMarkets()
function to generate a DoS with having many tokens.Proof of Concept
https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L772-L798 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L1060-L1062
Tools Used
Manual reading
Recommended Mitigation Steps
Add a withdraw() function you could remove the element from allMarkets. This would make the variable not grow without reducing elements.
Assessed type
DoS