Moonwell - Sathish9098's results

LOW FINDINGS

[L-1] _newEndTime should be in the future. _endTime > block.timestamp + 1 should be used instead of _newEndTime > block.timestamp

Impact

Using _newEndTime > block.timestamp to check if the new end time is in the future is not sufficient because it allows the new end time to be set to the current block timestamp, which is not what you'd want in most cases.To guarantee that _newEndTime is in the future, you should use the comparison _newEndTime > block.timestamp + 1 instead. This ensures that the new end time is at least one second greater than the current block timestamp, making it a valid future time.

FILE: Breadcrumbs2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

793: require(
794:            _newEndTime > block.timestamp,
795:            "_newEndTime MUST be > block.timestamp"
796:        );

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

Recommended Mitigation

Change the _newEndTime check same like _addEmissionConfig() function . This will ensures the valid future time.

require(
            _endTime > block.timestamp + 1,
            "The _endTime parameter must be in the future!"
        );

[L-2] The safe32(block.timestamp) returns truncated values after year 2106

Impact

After the year 2106, the block.timestamp will continue to increment normally, but the truncated timestamp variable will reset to zero after reaching its maximum value (2^32 - 1). As a result, the timestamp variable will behave as if it's cycling every 2^32 seconds (approximately 136 years), causing it to lose track of the actual time

FILE: 2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

436: supplyGlobalTimestamp: safe32(
                block.timestamp,
                "block timestamp exceeds 32 bits"
            ),

442: borrowGlobalTimestamp: safe32(
                block.timestamp,
                "block timestamp exceeds 32 bits"
            ),

919: uint32 blockTimestamp = safe32(
            block.timestamp,
            "block timestamp exceeds 32 bits"
        );

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

Recommended Mitigation

When even handling block.timestamp use at least uint128 to avoid unexpected behaviors

[L-3] Don't trust single oracle for price

Impact

Currently protocol only uses Chainlink Oracle for prices

You should not trust a single oracle for price calculations. There are a number of reasons why this is the case

• Single oracles can be attacked: If a single oracle is attacked, it could be compromised and provide incorrect price information. This could lead to significant losses for users who rely on the price information from the oracle.
• Single oracles can be unreliable: Even if a single oracle is not attacked, it could still be unreliable. This could be due to technical problems, such as network outages or hardware failures. It could also be due to human error.
• Single oracles can be biased: A single oracle could be biased towards providing certain price information. This could be due to a number of factors, such as the oracle's own financial interests or the interests of its owners.

Recommended Mitigation

Using multiple oracles for price calculations is to use a weighted average. A weighted average is a method of combining the price information from multiple oracles. The weight of each oracle is based on its reliability. This approach can help to ensure that the price information is accurate and reliable, even if some of the oracles are unreliable

[L-4] Lack of integer validations in _setEmissionCap() function leads unexpected behavior

Impact

Lack of integer validations in the _setEmissionCap() function can indeed lead to unexpected behavior and potential vulnerabilities due to human errors.Setting a very low emission cap unintentionally could limit the contract's functionality or prevent it from functioning as intended. It could also have adverse effects on token economics if the emission cap is set too low

POC

FILE: Breadcrumbs2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

function _setEmissionCap(
        uint256 _newEmissionCap
    ) external onlyComptrollersAdmin {
        uint256 oldEmissionCap = emissionCap;

        emissionCap = _newEmissionCap;

        emit NewEmissionCap(oldEmissionCap, _newEmissionCap);
    }

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

Recommended Mitigation

• Add grater than zero check

require(_newEmissionCap > 0, ``Not a zero ``);

• Add Min and Max Values check

require(_newEmissionCap > MIN && _newEmissionCap < MAX, ``Not a zero ``);

[L-5] Owner may set less total supply limit in updateMarketSupplyIndexInternal()

Impact

There is no explicit check for the total supply limit of the MToken (_mToken). This lack of validation could indeed lead to potential issues if the owner mistakenly sets the total supply to a value less than the actual circulating supply or if the total supply is incorrectly changed.

POC

FILE: Breadcrumbs2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

 function updateMarketSupplyIndex(
        MToken _mToken
    ) external onlyComptrollerOrAdmin {
        updateMarketSupplyIndexInternal(_mToken);
    }

function updateMarketSupplyIndexInternal(MToken _mToken) internal {
        MarketEmissionConfig[] storage configs = marketConfigs[
            address(_mToken)
        ];

        uint256 totalMTokens = MTokenInterface(_mToken).totalSupply();

        // Iterate over all market configs and update their indexes + timestamps
        for (uint256 index = 0; index < configs.length; index++) {
            MarketEmissionConfig storage emissionConfig = configs[index];

            // Go calculate our new values
            IndexUpdate memory supplyUpdate = calculateNewIndex(
                emissionConfig.config.supplyEmissionsPerSec,
                emissionConfig.config.supplyGlobalTimestamp,
                emissionConfig.config.supplyGlobalIndex,
                emissionConfig.config.endTime,
                totalMTokens
            );

            // Set the new values in storage
            emissionConfig.config.supplyGlobalIndex = supplyUpdate.newIndex;
            emissionConfig.config.supplyGlobalTimestamp = supplyUpdate
                .newTimestamp;
            emit GlobalSupplyIndexUpdated(
                _mToken,
                emissionConfig.config.emissionToken,
                supplyUpdate.newIndex,
                supplyUpdate.newTimestamp
            );
        }
    }

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

Recommended Mitigation

Consider implementing checks in the function to validate that the _mToken's total supply is within a reasonable range and not below the current circulating supply

uint256 currentTotalSupply = MTokenInterface(_mToken).totalSupply();
require(currentTotalSupply >= totalMTokens, "Invalid total supply: Total supply cannot be decreased.");

[L-6] MultiRewardDistributor contract inherited the Pausable.sol but not actually Pausable

Impact

MultiRewardDistributor inherits from Pausable.sol but does not actually implement the pausable functionality, it can lead to confusion and potential security risks. The Pausable pattern is used to add a pausing mechanism to a smart contract, allowing the contract owner to pause and unpause certain functions temporarily. It's used to handle emergency situations, bugs, or unexpected behavior.

POC

FILE: 2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

44: Pausable,

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

Recommended Mitigation

whenNotPaused can be used to pause and resume certain critical operations

[L-7] _emissionConfig.supplierIndices[_supplier] in some cases this will return default uint256 value 0

Impact

If the _supplier is a valid address but has not yet interacted with the contract or does not have an associated index in the mapping, accessing _emissionConfig.supplierIndices[_supplier] will return the default value for uint256, which is 0. Depending on how the contract uses this index, it may lead to incorrect calculations or undesired behavior.

POC

FILE: 2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

833: uint256 userSupplyIndex = _emissionConfig.supplierIndices[_supplier];

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

Recommended Mitigation

One way to do this is to use the .contains() function provided by a mapping library like OpenZeppelin's

[L-8] calculateSupplyRewardsForUser(), calculateBorrowRewardsForUser() functions does not explicitly check for the existence of the _supplier address in _emissionConfig.supplierIndices Mapping

Impact

If the _emissionConfig.supplierIndices mapping should only contain valid addresses that have been added beforehand, it might be necessary to add additional validation to ensure that _supplier is a valid address before accessing its index in the mapping. It directly accesses userSupplyIndex from the mapping, assuming that the address exists.

POC

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

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

Recommended Mitigation

Validate that the _supplier address exists in the set before accessing its index in the mapping

[L-9] There is no guarantee that the _user address can receive ETH

Impact

There is no guarantee that _user can receive ETH directly in the sendReward function, especially if _user is a contract address or an externally owned address that does not implement a fallback function to receive ETH.

Sending ETH directly to a contract or an externally owned address without a payable fallback function will result in a transaction failure, and the transfer will not be successful. There is no address(0) checks

Before sending ETH, you can check if _user is a contract address using the isContract function. If it's a contract, you can check if it has a payable fallback function to receive ETH. If it does, proceed with the ETH transfer. If it doesn't, handle the situation accordingly (e.g., log an event, revert the transaction, etc.).

POC

FILE: Breadcrumbs2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

1215: address payable _user,

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

Recommended Mitigation

sendEthReward function checks if _user is a contract and whether it has a payable fallback function before sending ETH.

[L-10] Using token.balanceOf(address(this)) every time this will reduce the contract performance

Impact

Using token.balanceOf(address(this)) every time can potentially reduce the contract's performance. Frequent external calls to other contracts, such as reading the balance of the token contract, can be expensive in terms of gas costs and execution time. This can lead to higher transaction costs and slower contract execution.

POC

FILE: Breadcrumbs2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

480:  token.balanceOf(address(this))

1232: uint256 currentTokenHoldings = token.balanceOf(address(this));

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

Recommended Mitigation

Consider State variables instead of token.balanceOf(address(this))

[L-11] Project has NPM Dependency which uses a vulnerable version : @openzeppelin

Impact

Project mostly uses openzeppelin 4.9.0. There is latest version available 4.9.3

There are known issues in this version

• Improper Input Validation
• Missing Authorization

POC

{ "name": "openzeppelin-solidity", "version": "4.9.0",

Recommended Mitigation

Use latest version v4.9.3

[L-12] Insufficient coverage

- What is the overall line coverage percentage provided by your tests?: 80%

Impact

The code base, the 80% line coverage percentage means that 80% of the lines of code in the contract have been executed by the tests. This is a good start, but it is not enough to ensure that the contract is fully tested.

Recommended Mitigation

Achieve 100% Percent lines coverage to avoid risks

[L-13] Update codes to avoid Unexpected behavior

There are warnings in the codebase. Try to rectify all warnings to avoid unexpected behaviors in protocol

Warning (2519): Warning: This declaration shadows an existing declaration.
   --> src/core/Governance/deprecated/MoonwellArtemisGovernor.sol:349:9:
    |
349 |         ProposalState state = state(proposalId);
    |         ^^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
   --> src/core/Governance/deprecated/MoonwellArtemisGovernor.sol:372:5:
    |
372 |     function state(uint proposalId) public view returns (ProposalState) {
    |     ^ (Relevant source part starts here and spans across multiple lines).


Warning (3628): Warning: This contract has a payable fallback function, but no receive ether function. Consider adding a receive ether function.
  --> src/core/MErc20Delegator.sol:11:1:
   |
11 | contract MErc20Delegator is MTokenInterface, MErc20Interface, MDelegatorInterface {
   | ^ (Relevant source part starts here and spans across multiple lines).
Note: The payable fallback function is defined here.
   --> src/core/MErc20Delegator.sol:496:5:
    |
496 |     fallback () external payable {
    |     ^ (Relevant source part starts here and spans across multiple lines).


Warning (2462): Warning: Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.
   --> src/core/Governance/deprecated/MoonwellArtemisGovernor.sol:259:5:
    |
259 |     constructor(
    |     ^ (Relevant source part starts here and spans across multiple lines).


Warning (2462): Warning: Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.
  --> src/core/MLikeDelegate.sol:19:3:
   |
19 |   constructor() public MErc20Delegate() {}
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


Warning (2072): Warning: Unused local variable.
  --> test/integration/LiveSystem.t.sol:66:9:
   |
66 |         uint256 startingTokenBalance = token.balanceOf(sender);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[L-14] Token transfer to address(0) should be avoided

Impact

The external function sendReward() ignores the use of address(0).Need to check address(0) even when using safeTransfer. safeTransfer is a function that is designed to prevent certain types of errors from occurring when transferring tokens. However, it does not protect against all possible errors.

POC

FILE: 2023-07-moonwell/src/core/MultiRewardDistributor/MultiRewardDistributor.sol

function sendReward(
        address payable _user,
        uint256 _amount,
        address _rewardToken
    ) internal nonReentrant returns (uint256) {
        // Short circuit if we don't have anything to send out
        if (_amount == 0) {
            return _amount;
        }

        // If pause guardian is active, bypass all token transfers, but still accrue to local tally
        if (paused()) {
            return _amount;
        }

        IERC20 token = IERC20(_rewardToken);

        // Get the distributor's current balance
        uint256 currentTokenHoldings = token.balanceOf(address(this));

        // Only transfer out if we have enough of a balance to cover it (otherwise just accrue without sending)
        if (_amount > 0 && _amount <= currentTokenHoldings) {
            // Ensure we use SafeERC20 to revert even if the reward token isn't ERC20 compliant
            token.safeTransfer(_user, _amount);
            return 0;
        } else {
            // If we've hit here it means we weren't able to emit the reward and we should emit an event
            // instead of failing.
            emit InsufficientTokensToEmit(_user, _rewardToken, _amount);

            // By default, return the same amount as what's left over to send, we accrue reward but don't send them out
            return _amount;
        }
    }

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

Recommended Mitigation

Add address(0) check before calling safeTransfer() function

require(_user!=address(0), "Zero Address");

[L-15] Deprecated Comment

Impact

The comment ```/// @dev this branch should never get called as native tokens are not supported on this deployment`` is outdated and does not match the actual implementation. As mentioned earlier, nativeToken is not used in the contract, so the branch is indeed getting called and can be problematic.

POC

FILE: 2023-07-moonwell/src/core/Oracles/ChainlinkOracle.sol

62: if (keccak256(abi.encodePacked(symbol)) == nativeToken) { /// @dev this branch should never get called as native tokens are not supported on this deployment

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Oracles/ChainlinkOracle.sol#L62

[L-16] Remove the nativeToken variable

This variable is no longer needed, and it could be removed from the contract

 /// @param _nativeToken The native token symbol, unused in this deployment so it can be anything

FILE: 2023-07-moonwell/src/core/Oracles/ChainlinkOracle.sol

46: nativeToken = keccak256(abi.encodePacked(_nativeToken));

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Oracles/ChainlinkOracle.sol#L46

[L-17] The setDirectPrice function should be modified to check the validity of the price before setting it

This code first checks that the price is greater than zero. It then checks that the price is less than 1e18. If both of these checks pass, the price is set and the PricePosted event is emitted

- The price should be greater than zero.
- The price should be a valid decimal number.
- The price should not be too large or too small.

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Oracles/ChainlinkOracle.sol#L135-L138

Analysis - Moonwell Protocol

Summary

Overview

Moonwell Protocol is a fork of Compound v2 with features.

Moonwell is an open lending and borrowing protocol built on Base, Moonbeam, and Moonriver.The protocol's intuitive design ensures a seamless user experience, enabling users to easily navigate through various features and perform operations effortlessly.

Important Contracts:

• ChainlinkCompositeOracle
  • which aggregates mulitple exchange rates together
• MultiRewardDistributor
  • Allow distributing and rewarding users with multiple tokens per MToken. Parts of this system that require special attention are what happens when hooks fail in the Comptroller
• Comptroller
  • This contract is based on the Flywheel logic
• TemporalGovernor
  • which is the cross chain governance contract. Specific areas of concern include delays, the pause guardian, putting the contract into a state where it cannot be updated

Special features of Moonwell Protocol

• Non-Custodial
• User Experience
• Security
• Transparency
• Onchain Governance

My Thoughts

Moonwell Protocol may changes Lending and Borrowing to next level

Moonwell's approach to lending and borrowing in DeFi can be a catalyst for positive changes in the ecosystem. By setting a standard for user experience, security, transparency, and governance, it may inspire other projects to improve and innovate, leading to a more mature, inclusive, and trustworthy DeFi lending and borrowing landscape in the future.

Decentralization and Governance: Moonwell's onchain governance model can pave the way for more decentralized decision-making processes in other DeFi projects. As protocols adopt similar governance mechanisms, the community's voice and participation may play a more significant role in shaping the future of DeFi platforms.

Trust and Transparency: The emphasis on transparency in Moonwell's onchain operations can foster trust among users. As other protocols adopt similar transparency practices, users may feel more confident in interacting with DeFi platforms, leading to increased trust within the ecosystem.

Non-Custodial Solutions: Moonwell's non-custodial approach to lending and borrowing may become a trend in the DeFi space. More protocols might adopt non-custodial models to allow users to retain control over their assets, aligning with the core principles of DeFi.

Global Reach: Moonwell's focus on accessibility and usability may lead to increased global adoption of DeFi lending and borrowing. As the protocol reaches users in different regions, DeFi's impact could become more widespread and inclusive.

Audit approach

I followed below steps while analyzing and auditing the code base.

1. Read the contest Readme.md and took the required notes.

• Moonwell Protocol protocol uses
• inheritance
• Tests only covered 80%
• Multi-Chain
• ERC-20 Token
• Timelock function
• Need to understand moonbeam and temporal governance to understand the governance system
• Uses Chainlink price oracle
• This protocol if fork of Compound with MRD

2. Analyzed the over all codebase one iterations very fast

3. Study of documentation to understand each contract purposes, its functionality, how it is connected with other contracts, etc.

4. Then i read old audits and already known findings. Then go through the bot races findings

5. Then setup my testing environment things. Run the tests to checks all test passed. I used foundry to test moonwell protocol. I used forge comments for testing

6. Finally, I started with the auditing the code base in depth way I started understanding line by line code and took the necessary notes to ask some questions to sponsors.

Stages of audit

• The first stage of the audit

During the initial stage of the audit for Moonwell Protocol, the primary focus was on analyzing gas usage and quality assurance (QA) aspects. This phase of the audit aimed to ensure the efficiency of gas consumption and verify the robustness of the platform.

Found 14 QA Findings

• The second stage of the audit

In the second stage of the audit for Moonwell Protocol, the focus shifted towards understanding the protocol usage in more detail. This involved identifying and analyzing the important contracts and functions within the system. By examining these key components, the audit aimed to gain a comprehensive understanding of the protocol's functionality and potential risks.

• The third stage of the audit

During the third stage of the audit for Moonwell Protocol, the focus was on thoroughly examining and marking any doubtful or vulnerable areas within the protocol. This stage involved conducting comprehensive vulnerability assessments and identifying potential weaknesses in the system. Found 60-70 vulnerable and weakness code parts all marked with @audit tags.

• The fourth stage of the audit

During the fourth stage of the audit for Moonwell Protocol, a comprehensive analysis and testing of the previously identified doubtful and vulnerable areas were conducted. This stage involved diving deeper into these areas, performing in-depth examinations, and subjecting them to rigorous testing, including fuzzing with various inputs. Finally concluded findings after all research's and tests. Then i reported C4 with proper formats

Found one medium risk findings admin may receive less token than expected because of this check _amount == type(uint256).max

Learnings

The Moonwell Protocol team can draw lessons from other DeFi protocols that have faced similar challenges. Studying security incidents, best practices, and successful governance mechanisms from other projects can provide valuable insights for building a more robust and resilient platform.

By prioritizing security, conducting thorough risk assessments, and actively involving the community in governance decisions, the Moonwell Protocol can proactively address these areas of concern and strengthen its position as a secure and community-driven DeFi lending and borrowing platform

Possible Systemic Risks

• Smart Contract Vulnerabilities: The use of inheritance and integration of multiple features can introduce potential smart contract vulnerabilities. Bugs, logic flaws, or improper implementations could lead to exploits and financial losses.

• Test Coverage Risks: With only 80% test coverage, the protocol may have undiscovered vulnerabilities. Inadequate security audits could leave critical issues unnoticed, increasing the risk of attacks

• Governance Challenges: Understanding Moonbeam and Temporal Governance systems may be complex for some users, potentially leading to governance inefficiencies or suboptimal decision-making.

• Chainlink Oracle Risks: Relying on a single oracle provider, like Chainlink, exposes the protocol to potential risks associated with oracle failures, data manipulation, or centralization issues.

• Protocol Fork Risks: Forking from other protocols may inherit vulnerabilities present in the original codebase. Failure to address these risks could result in potential attacks.

• Liquidity and Market Risks: Insufficient liquidity or market manipulation risks can affect the protocol's stability and overall performance.

Code Commentary

• Use consistant SPDX-License for all contracts

• Inconsistance solidity versions used

• Ensure that variable and function names follow consistent naming conventions for better readability and maintainability. For example, use camelCase or snake_case consistently throughout the contract

• In the Status and DistributionStatus structs, consider using an enum type for stage to improve readability and avoid potential bugs caused by using numbers directly

• Some functions like updateMarketSupplyIndexAndDisburseSupplierRewards and updateMarketBorrowIndexAndDisburseBorrowerRewards are similar. You can consolidate them into a single function that takes an additional parameter to specify the action (supplier rewards or borrower rewards)

• Instead of importing the whole contract code for IERC20 and MToken, use interfaces to define only the required functions. This reduces the gas cost and enhances code readability

• Add inline comments to explain complex logic, especially in mathematical calculations, to make the code more understandable

• Add appropriate error messages in require statements to provide more informative feedback to users when a transaction fails

• In some functions like exitMarket, multiple emit statements are used for the same event. Consolidate the event emissions to a single location to reduce code duplication and improve readability.

• It's a good practice to add more detailed information in the event logs, such as the account addresses involved in certain actions.

• Some functions return error codes (e.g., uint(Error.NO_ERROR)), but the exact meaning of these error codes is not explicitly defined. Proper error code documentation or the use of error enums would enhance clarity and usability.

• In the addressToBytes function, you can use bytes20(addr) directly instead of casting it as a bytes20. It will not have any impact on gas costs, but it simplifies the code

• In the setTrustedSenders and unSetTrustedSenders functions, avoid encoding and decoding the same data by directly using the addr parameter as the key for the mapping

• Emit events for significant contract state changes, as it helps in tracking contract activities and provides transparency to external applications

• Consider refactoring the code to break down complex functions into smaller, more manageable pieces, following the principles of modularity

• Instead of using separate bool variables like guardianPauseAllowed, you can use an enum to represent different contract states, such as enum ContractState { Active, Paused, GuardianRevoked }. This can help make the contract state more explicit and easier to understand

Centralization risks

A single point of failure is not acceptable for this project Centrality risk is high in the project, the role of onlyOwnerdetailed below has very critical and important powers

Project and funds may be compromised by a malicious or stolen private key onlyOwner msg.sender

File: src/core/Governance/TemporalGovernor.sol

266:     function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {

274:     function togglePause() external onlyOwner {

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

Gas Optimizations

1. Using token.balanceOf(address(this)) every time can potentially reduce the contract's performance. Frequent external calls to other contracts, such as reading the balance of the token contract, can be expensive in terms of gas costs and execution time. This can lead to higher transaction costs and slower contract execution

2. Explicitly initializing default values for variables consumes additional gas during contract deployment. If you rely on the EVM's automatic default initialization, you can save gas costs associated with these unnecessary operations

3. Minimize the number of state variable reads and writes. Use local variables when possible to avoid additional storage operations

4. Limit the number of iterations in loops to prevent excessive gas consumption. Consider using other patterns like mapping, where possible, to reduce the need for loops

5. Whenever applicable, consider batching multiple operations together to save on gas costs. For example, you can combine multiple token transfers into a single function call

6. Remove any unused or commented-out code to save space and simplify contract logic

7. Check if modifiers can be combined or reused across multiple functions to reduce the number of modifier calls. This can save gas by avoiding redundant checks.

8. The enterMarkets function currently loops through the list of mTokens and emits an event for each entry. Consider batching the events or using other gas-saving techniques to reduce transaction costs

9. In the addToMarketInternal function, you can optimize the code by storing markets[address(mToken)] in a local variable, rather than repeatedly accessing it within the function

10. Some operations, such as updating supply and borrow indexes, are performed in a loop for each market in the claimReward function. This could lead to exceeding the block gas limit if the number of markets is substantial

11. Evaluate whether certain loops can be simplified or avoided altogether to reduce gas costs. For instance, consider whether iterating over the allMarkets array in the _addMarketInternal function can be optimized

12. Reuse variables instead of creating new ones when possible. This can reduce memory usage and, in turn, save gas

13. For functions that don't modify state, use the view or pure modifiers to avoid unnecessary gas costs associated with state changes

14. For small and simple functions, consider inlining them within other functions to save gas costs associated with function calls

15. Use gas-efficient math libraries, if available, to perform arithmetic operations. Check if there are any gas-efficient replacements for the current math operations

16. If certain variables, such as admin, borrowCapGuardian, pauseGuardian, etc., are not intended to be changed after deployment, mark them as immutable to enhance security and gas efficiency

17. Simplify code logic and eliminate redundant calculations

18. In the executeProposal function, there are two function calls to trustedSenders[vm.emitterChainId].contains(vm.emitterAddress). Consider storing the result of this call in a variable and using that variable to avoid calling the function twice

19. In the setTrustedSenders and unSetTrustedSenders functions, you can move the msg.sender == address(this) check outside the loop to minimize storage access in each iteration saves gas

20. In the constructor, you can use emit to emit the TrustedSenderUpdated event instead of using emit in the loop. This can reduce the gas cost by avoiding multiple event emissions

21. In the executeProposal function, the return value returnData is not used. You can remove it if it's not required.

Risks as per Analysis

• Lack of integer validations in _setEmissionCap() function leads unexpected behavior

• if (_amount == type(uint256).max) check is wrong . There is no gurantee that type(uint256).max and contract balance or same

• updateMarketSupplyIndexInternal(),updateMarketSupplyIndexInternal() there is no limits checked when updating totalsupply

• Inline assembly should be used with caution as it can introduce security risks. Whenever possible, prefer using built-in Solidity functions and libraries.

MultiRewardDistributor.sol

• The contract interacts with external contracts, such as comptroller, IERC20, and MToken, through external calls. These external calls are not checked for success or failure, and there is no handling for potential exceptions or revert reasons, which can lead to unexpected results.

• The contract involves various mathematical calculations, such as index updates and reward distributions. It's crucial to ensure that these operations are handled correctly and do not result in integer overflows or underflows.

• Some of the functions in the contract, such as updateMarketSupplyIndexInternal, updateMarketBorrowIndexInternal, disburseSupplierRewardsInternal, and disburseBorrowerRewardsInternal, perform calculations and iterations that could potentially consume a significant amount of gas. It's essential to ensure that these functions are optimized to prevent gas limitations and high transaction costs.

• The contract uses uint224 data type for some of the index values (_globalSupplyIndex, _globalBorrowIndex, etc.). If these indices grow too large, it could potentially lead to overflow issues and incorrect ``reward calculations```.

• The sendReward function performs a token transfer and is marked as nonReentrant, but it's essential to ensure that all external contract calls, especially token transfers, are safe against reentrancy attacks

• The contract allows the owner to update supply and borrow emission speeds. There should be careful consideration of the emission rates and their effects on the overall token economy to avoid unintended consequences

• The contract has two functions (calculateSupplyRewardsForUser and calculateBorrowRewardsForUser) for calculating rewards for suppliers and borrowers, respectively. Ensure that both functions use consistent calculations and share the same index values to prevent discrepancies

• Consider using upgradeable contract patterns to allow for future updates without redeploying the entire contract

Comptroller.sol

• In the transferAllowed function, there's a comment mentioning the use of local vars to avoid stack-depth limits in calculating account liquidity. While it's a good practice to use local variables for complex calculations, it's important to ensure that the overall stack depth is managed appropriately, especially in functions that could be called in nested scenarios

• The contract appears to have some administrative functions like _setPriceOracle, _setCloseFactor, _setCollateralFactor, etc., which should only be callable by the admin. However, the access control checks are not consistently applied in all these functions, potentially allowing unauthorized users to modify critical parameters

• Several functions call external contracts, like the rewardDistributor, oracle, and token contracts, but they do not have proper error handling for potential failures. Lack of error handling can lead to unexpected behavior and vulnerabilities

• The contract has several functions for pausing different actions (_setMintPaused, _setBorrowPaused, _setTransferPaused, _setSeizePaused). However, there is no mechanism for time-based unpausing or emergency unpause, which could be essential for the contract's safety

• The liquidateCalculateSeizeTokens function, used during liquidation, performs arithmetic calculations using external data (e.g., exchangeRateMantissa). Failing to handle or validate external data correctly could lead to vulnerabilities in the liquidation process

• The claimReward function, responsible for distributing rewards, has a complex design with nested loops, and it calls an external contract multiple times. High complexity increases the risk of errors and makes the contract harder to audit

• The getBlockTimestamp function retrieves the current block timestamp using block.timestamp. It's worth noting that relying solely on block timestamps for time-sensitive operations can be manipulated by miners to some extent

• Certain functions that modify contract parameters (e.g., _setPriceOracle, _setCloseFactor, etc.) do not include time limitations for changes. Adding time restrictions for critical parameter modifications could prevent malicious or mistaken changes

TemporalGovernor.sol

• The contract relies on trusted emitters from the Wormhole bridge using isTrustedSender. However, relying solely on a chain ID and a 32-byte emitter address might not be sufficient to ensure the authenticity of the signed messages. Consider using a more robust signature verification mechanism to ensure the validity of messages

• The contract doesn't have any explicit reentrancy protection. Ensure that critical state changes are performed before any external calls to prevent potential reentrancy attacks

• The contract uses uint248 for lastPauseTime, which might be susceptible to integer overflow if the pause timestamp becomes larger than the maximum value

• The fastTrackProposalExecution function allows the owner to bypass the usual delay for proposal execution. This can be risky as it doesn't have any checks on the validity of the proposal. It should be handled with caution and only used in specific emergency scenarios

• The function allTrustedSenders iterates through the entire trustedSenders set to return the list of trusted senders for a chain. This operation may become inefficient as the size of the set grows. Consider alternative data structures or optimizations if needed

ChainlinkCompositeOracle.sol

• The contract relies on Chainlink oracles to provide accurate price data. If a Chainlink oracle is compromised or provides incorrect data, the ChainlinkCompositeOracle contract could also be compromised

• The contract does not have any mechanisms in place to prevent front-running or other forms of market manipulation. This could allow malicious actors to exploit the contract and profit at the expense of other users

• The contract relies on the decimals value returned by Chainlink oracles. If the decimals value is misaligned with the contract's expectations, it could result in incorrect price calculations and pose a vulnerability

Non-functional aspects

• Aim for high test coverage to validate the contract's behavior and catch potential bugs or vulnerabilities
• The protocol execution flow is not explained in efficient way.
• Its best to explain over all protocol with architecture is easy to understandings
• Consider designing the contract to be upgradable or allow for versioning. This can help address issues, introduce new features, or adapt to evolving requirements without disrupting the entire system

Time spent on analysis

15 Hours

Time spent:

13 hours