Moonwell - berlin-101's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L217

Vulnerability details

Impact

Revoking the guardian of TemporalGovernor.sol contract does unpause the contract as an unexpected side effect which may put funds at risk.

Proof of Concept

When revoking the guardian of TemporalGovernor.sol via the "revokeGuardian" of function (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L205) the contract is also automatically unpaused (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L217).

According to docs in code "Revoking the guardian can only be done via governance proposal or by the guardian itself." And it is further documented in code that the function "unpauses the contract if paused": (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L217)

Nevertheless, it cannot be assumed that a guardian or someone creating a proposal to revoke the guardian (most likely both done via UI) is familiar with the code and the function comments. Therefore he may be unaware that his "revoke the guardian" action instantaneously unpauses the contract. There are two facts that further support this assumption:

1. The function is only called "revokeGuardian" and not "revokeGuardianAndUnpause" which hides its unpausing side effect.
2. The more high-level, and lot more user-friendly documentation for the "revokeGuardian" function does not state the unpausing side effect (vs. documentation in code): "Revokes the guardian's ability to pause the contract and transfers ownership to address(0)." (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/docs/TEMPORALGOVERNOR.md?plain=1#L59).

Due to the combination of missing transparency for users in regards to the "unpause" side-effect of the "revokeGuardian" function and the importance of a controlled pause/unpause of the TemporalGovernor.sol contract, which if done wrong, may put funds at risk when contract assumptions (https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L13-L26) are broken, this is considered more than just a QA issue.

Tools Used

Manual review

Recommended Mitigation Steps

• Update documentation to be in sync with the comments in the code
• Rename function from "revokeGuardian" to "revokeGuardianAndUnpause"
• Optionally (and this would be a nice feature extension), add another parameter to the affected function. If it is set to true, execute as currently implemented. If set to false, don't reset the "lastPauseTime" state variable. The contract will stay paused and can be unpaused by anyone via "permissionlessUnpause" once the "permissionlessUnpauseTime" has passed.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L194

Vulnerability details

Impact

Calling grantGuardiansPause() on TemporalGovernor.sol can brick governance that can only be escaped by revoking the guardian, which removes contract pauseability.

Proof of Concept

1. Governor calls togglePause() when contract is not paused: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L274 In that function call "guardianPauseAllowed" is set to false: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L283.

2. The governor could call togglePause() again to unpause which would happen here: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L276 and would succeed as "guardianPauseAllowed" is still false: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L289.

3. grantGuardianPause() gets called: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L188 In that function call "guardianPauseAllowed" is set to true: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L194.

4. When the guardian now calls togglePause() to unpause the contract, it fails since the "guardianPauseAllowed" will make the call fail here: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L289.

5. Also, this will fail any call to "permissionlessUnpause" to unpause the contract here: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L256.

In consequence, while the contract is still in a paused state, this would leave calling revokeGuardian() as the only option to unpause the contract: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L217, because it does not evaluate the "guardianPauseAllowed" state variable in its logic.

Once revokeGuardian() was called, no pausing can ever be done to the contract since pausing is only allowed for the guardian (the contract owner, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L285), which was revoked.

This order of calls can certainly happen since it is not prevented by the implementation, which would force revoking the guardian to get the contract unpaused, which also removes the contract pauseability. The option to pause the contract if assumptions are broken (see: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L13-L26) is critical to protect funds. Losing this essential security mechanism will put funds at risk.

Tools Used

Manual review

Recommended Mitigation Steps

Check within "guardianPauseAllowed" whether the contract is still paused, and don't allow the action if it is still paused. Alternatively, within the "permissionlessUnpause" function, reset the "guardianPauseAllowed" to true and don't have a "guardianPauseAllowed" function. Then the conflict that causes the issue cannot be created. Rethink whether it is meaningful to allow resetting the guardian's allowance for pausing while the contract is still in paused state.

Assessed type

Governance

L-01 Checks for zero address in _acceptImplementation and _acceptAdmin functions are unnecessary

A message can never be sent by the zero address and both functions. The first check of both statements that include the check for the zero address would already fail in case the zero address was set as pending. https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Unitroller.sol#L61, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Unitroller.sol#L111. The check for the zero address can be removed.

L-02 Using underscore prefix for external/public functions

Bot findings highlight that "Non-external/public variable and function names should begin with an underscore". But in the codebase, there is also underscores used as a prefix for external/public functions. Examples: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20.sol#L160, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20Delegator.sol#L412. This should be cleaned up.

L-03 Same code blocks have different indentations for comments in MErc20.sol

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20.sol#L194-L202 and https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20.sol#L194-L202 that are identical but have different indentations for comments. This should be cleaned up.

L-04 Typo in MErc20Delegator.sol

It should be "setter" instead of "settor": https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20Delegator.sol#L48.

L-05 Development comments should be removed from codebase

Here is one example: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L71-L72. This should be documented in accompanying documents but not be left in code.

L-06 TemporalGovernor.sol should potentially also emit TrustedSenderUpdated events for setting the trusted senders in the constructor

Setting and unsetting trusted senders via the respective functions of TemporalGovernor.sol emits TrustedSenderUpdated events (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L151-L154, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L178-L181). The constructor does not (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L74-L78), but could to have complete transparency from the start, who the trusted senders are (e.g. for off-chain risk monitoring).

L-07 Unnecessary check for _amount > 0 in sendReward function of MultiRewardDistributor.sol

There is already a check that returns 0 if _amount is equal to 0 (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1220). The datatype (uint256) does not allow negative values so the additional check for _amount > 0 (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1235) is unnecessary and can be removed.

L-08 Unnecessary cast of comptroller.admin() to address in MultiRewardDistributor.sol

Here a cast is done to address: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L126. The same check is done a few lines lower without casting to address: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L136. The cast to address can be removed for keep consistency.

L-09 "Set the new values in storage" comments within view functions of MultiRewardDistributor.sol

The following comments should be removed as it is only view functions: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L348, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L363.

L-10 Inconsistency between contracts how timestampsPerYear constant is encoded

JumpRateModel.sol represents the state variable as an integer multiplication: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/IRModels/JumpRateModel.sol#L20. WhitePaperInterestRateModel.sol represents the constant as a single integer (multiplication result): https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/IRModels/WhitePaperInterestRateModel.sol#L20. The same representation should be used across all contracts for consistency. The integer multiplication should be preferred.

L-11 Typo in WhitePaperInterestRateModel

It should be "timestamp" instead of "timestmp": https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/IRModels/WhitePaperInterestRateModel.sol#L61, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/IRModels/WhitePaperInterestRateModel.sol#L65.

L-12 setTrustedSenders and unSetTrustedSenders of TemporalGovernor.sol should only emit events for actually added/removed trusted senders.

The add() and remove functions of EnumerableSet return true if actually a change to the set was made (https://docs.openzeppelin.com/contracts/4.x/api/utils#EnumerableSet-add-struct-EnumerableSet-Bytes32Set-bytes32-, https://docs.openzeppelin.com/contracts/4.x/api/utils#EnumerableSet-remove-struct-EnumerableSet-Bytes32Set-bytes32-. But the logic of TemporalGovernor.sol does not evaluate these return values and always emits the events (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L151-L155, https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L178-L182). This should be corrected, and only when the add() or remove() functions returns true, the respective event should be emitted. With the current implementation, off-chain monitoring cannot differentiate whether a trusted sender was actually added or removed from the set.

L-13 calculateSupplyRewardsForUser() and calculateBorrowRewardsForUser() functions of MultiRewardDistributor.sol contains commented-out critical code:

calculateSupplyRewardsForUser() contains:

userSupplyIndex = initialIndexConstant; //_globalSupplyIndex;

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L839

calculateBorrowRewardsForUser() contains:

userBorrowIndex = initialIndexConstant; //userBorrowIndex = _globalBorrowIndex;

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L878

The commented-out code should be removed. It could get changed again and make it into production accidentally, which would change the accounting logic.

Change the error messages to the following to be more differentiated according to the "<" used for the check (>= should fail) and harmonize them regarding their wording (e.g., Cannot vs. Can't):

L-14 Error messages in MultiRewardDistributor.sol regarding supply/borrow emissions and supply speed are copy pasted and, in parts, incorrect

• https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L401 should be: "Cannot set supply emissions per second equal to or higher than the emission cap!"
• https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L405 should be: "Cannot set borrow emissions per second equal to or higher than the emission cap!"
• https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L676 should be: "Cannot set supply speed equal to current supply speed!"
• https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L680C13-L680C77 should be: "Cannot set supply speed equal to or higher than the emission cap!"

L-15 Wrong reference of "delegator" in _setImplementation() comments where it should be "delegate" in MErc20Delegator.sol

The comment should be "Called by the admin to update the implementation of the delegate" instead of "Called by the admin to update the implementation of the delegator" (https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20Delegator.sol#L56).

1. Approach taken in evaluating the codebase

• Sorted the contracts by lines of code and went through them, starting with the contracts having the lowest lines of code.
• Starting with small contracts allowed me to mentally warm up and get some first context before diving into bigger contracts.
• Did a line-by-line review of each contract and left audit markers for each interesting finding (issue candidates, QA findings, etc.) to report later.
• When approaching a contract, I read the documentation .md file to get context if available.
• Checked the bigger contracts, like Comptroller.sol, that were in audit scope by downloading the sources of the forked contracts (from Compound and Benqi) and did a diff using VSCode to see the parts that would need my attention.
• Watched the walkthrough videos that were available on YouTube for additional context.
• Did a deep-dive on MultiRewardDistributor.sol by copying the file and removing all logic that I completely understood and did not find issues in to leave only the parts containing accounting logic. Allowed me to focus better. I could still jump back to the original file to occasionally look up pieces I had removed.
• Once in a while, when I was losing focus, I collected the findings and reported them to have a constant flow of reporting findings. I learned from previous contests that, in the end, time could be tight, so I made sure to now wait with reports until the final day.

2. Architecture recommendations

• The approach with the MErc20Delegator.sol and MErc20Delegate.sol felt a bit odd. I have done a few audits already, and this kind of setup, where I contract basically repeats all the functions of the other and delegates them, I have not seen before. There potentially is a more elegant approach to this. To better judge this, I would have needed to dive deeper into the proxy/upgradeable setup of the protocol, which I did not focus on.

• The Chainlink Oracle implementation does not follow Compound's expected way of error handling (returning code 0 instead of reverting in some cases). Since the project is heavily based on Compound logic, I would suggest conforming to Compound standards. I checked all the places where the Oracle is used, and in every case, the 0 return code was handled correctly with an Error.PRICE_ERROR.

• The approach to map the symbol of an underlying token for a market (mToken) I would suggest reviewing it. It is not guaranteed by EIP-20 standards that a token returns a symbol, and it is also possible that a token's symbol will arbitrarily change, which would break the mapping/lose the oracle until it is fixed. I encourage you to rethink this design decision and replace it with an address-to-address mapping (token address -> Chainlink price feed address).

• I would suggest documenting what an "index" is in the context of the accounting math and why you changed from the Compound way of accounting to using "emissions per second". Please also extend the very critical MultiRewardDistributor.Sol contract with a lot more inline comments. This will allow an auditor to quicker grasp the accounting logic and spend more time on breaking things than on contract comprehension. In MToken.sol, this is way better. Generally, it would be very beneficial if the accounting logic was laid out in writing (e.g., in a whitepaper).

• Having an admin controlled overwrite for retrieving a price via the Chainlink Oracle and making the admin price the default case was discovered the first time within Moonbeam. It was not seen in previous audits, and nearly every protocol used a Chainlink oracle. Please rethink if you cannot invert this to use the Chainlink oracle first and fall back to the price set by admin.

3. Codebase quality analysis

• Generally, the codebase feels quite mature
• There is some commented-out logic in critical places (e.g., calculateSupplyRewardsForUser(), calculateBorrowRewardsForUser() of MultiRewardDistributor.sol) that need to be cleaned up. There is a risk they make it into the final code by accident.
• More inline comments in critical contracts (especially MultiRewardDistributor.sol) are missing
• Unit test coverage of 80% is a bit on the lower end and needs improvement
• Fuzz testing is used, which is positive
• Types are used a bit inconsistently (e.g., uint vs. uint256). Setting some standards and enforcing them (through static analysis, pre-commit hooks) would be great.
• Some leftovers of copy-pasting (e.g., comments of setter functions in getter functions that don't alter state variables) can be found and should be cleaned up
• In some places, checks for the 0 address are done as a second check within a require statement, but the zero address can never happen as the caller can never be the zero address. These things give a bit of the impression that the conditions were not thought through completely, which diminishes the feeling of a mature codebase a bit.
• There are no signs of static analysis tools being used for the protocol (e.g., slither). This should be done. Also, the codebase should be more rigorously be checked for typos. Both the audit contest botrace as well as the manual review of the codebase found typos.

4. Insights & Learnings

• In this audit it was attempted to break the TemporalGovernor.sol by moving mTokens between calls of claimReward(address holder, MToken[] memory mTokens) in Comtroller.sol. The idea was that the amount of rewards granted to an account is influenced by mToken.balanceOf(_supplier). The "testDifferentNumOfSuppliers" from MultiRewardDistributor.t.sol was modified to attempt a proof of concept. "forge-std/console.sol" was used to log the rewards being distributed, but the results did not show a sign this could be exploited. It looks like the custom "transferToken" implementation in mToken.sol makes sure that the rewards are distributed before moving the tokens to the new address.

5. Centralization risks

• The protocol is not completely permissionless, which imposes centralization risk
• Critical contracts either have an admin and/or additional parties granted critical rights (governor, guardian)
• It is important to ensure these parties are incentivized to behave in the users'/protocol's favor (game theory)
• It is important to not use individual accounts but multi sigs with a high enough threshold/quorum

Time spent:

32 hours