code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

`supplyCapGuardian` and `borrowCapGuardian` can abuse caps to prevent users from entering markets #217

Closed code423n4 closed 11 months ago

code423n4 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L844 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L807

Vulnerability details

Impact

An admin or cap guardian are able to set their caps to any arbitrary value without any timelock or restrictions. This can be abused by a malicious admin or guardian to prevent certain users from joining (or borrowing from) particular markets using frontrunning.

I believe medium severity is suitable as it can prevent users from being able to use the protocol but assumes we have a malicious guardian or admin

Proof of Concept

  1. Alice is set as the new supply guardian for Comptroller.sol through _setSupplyCapGuardian()
  2. Bob tries to start lending in the DAI market place by calling mDAI.mint()
  3. However, Alice frontruns Bob and sets supplyCap to 1
  4. mDai.mint() will call comptroller.mintAllowed() to ensure that all checks pass. However, as supplyCap != 0, unless there is no available funding in mDAI, then the mintAmount + cash + borrows - reserves < totalSupplyCap will fail thereby preventing Bob from joining the market
  5. Alice can then reset the supply cap to its original value

Tools Used

VSCode

Recommended Mitigation Steps

Consider adding a timelock to the supply caps with a queue and execute system or adding a minimum value (i.e. the current supply of the market) so that certain users cannot be hazed.

_queueMarketSupplyCaps(MToken[] calldata mTokens, uint[] calldata newSupplyCaps) external {
    ... // Sanity and admin checks
    queueTime = block.timestamp;
    pendingMToken = mTokens;
    pendingSupplyCaps= new SupplyCaps;
}

_executeMarketSupplyCaps() external {
    ... // Sanity and admin checks
    require(queueTime > 0, "No caps have been queued");
    require(queueTime + 2 hours <= block.timestamp, "Cannot change supply caps");
    for (uint i = 0; i < pendingMTokens.length; i++) {
        supplyCaps[address(pendingMTokens[i])] = pendingSupplyCaps[i];
    }
    queueTime = 0;

}

Assessed type

DoS

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as primary issue

ElliotFriedman commented 11 months ago

guardians are trusted, non issue

c4-sponsor commented 11 months ago

ElliotFriedman marked the issue as sponsor disputed

alcueca commented 11 months ago

The damage is minimal, and the obvious mitigation of replacing the malicious guardians obvious. Not even worth it to add in the docs.

c4-judge commented 11 months ago

alcueca marked the issue as unsatisfactory: Overinflated severity