Moonwell - kodyvim's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkCompositeOracle.sol#L180 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L97

Vulnerability details

Impact

wrong price could be returned in the event of a market crash.

Proof of Concept

From chainlink:

The data feed aggregator includes both minAnswer and maxAnswer values. These variables prevent the aggregator from updating the latestAnswer outside the agreed range of acceptable values, but they do not stop your application from reading the most recent answer.

Configure your application to detect when the reported answer is close to reaching minAnswer or maxAnswer and issue an alert so you can respond to a potential market event. Separately, configure your application to detect and respond to extreme price volatility or prices that are outside of your acceptable limits.

Chainlink oracles have a min and max price that they return. If the price goes below the minimum price the oracle will not return the correct price but only the min price. Same goes for the other extremity. Both ChainlinkCompositeOracle.getPriceAndDecimals and ChainlinkOracle.getChainlinkPrice does not check if price within the correct range: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L97

function getChainlinkPrice(
        AggregatorV3Interface feed
    ) internal view returns (uint256) {
        (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
            .latestRoundData();
        require(answer > 0, "Chainlink price cannot be lower than 0");
        require(updatedAt != 0, "Round is in incompleted state");

        ...SNIP
    }

wrong price may be returned in the event of a market crash.

Tools Used

Manual Review

Recommended Mitigation Steps

Check the latest answer against reasonable limits and/or revert in case you get a bad price

 require(answer >= minAnswer && answer <= maxAnswer, "invalid price");

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Calls to target with non-zero value would always revert.

Proof of Concept

Some external functions within TemporalGovernor uses _executeProposal to execute proposals but these external functions are not marked payable and contract does not implement recieve payable fallback. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L400 For instance:

 function executeProposal(bytes memory VAA) public whenNotPaused {
        _executeProposal(VAA, false);
    }

In _executeProposal: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L400

 function _executeProposal(bytes memory VAA, bool overrideDelay) private {
        
	...SNIP

        _sanityCheckPayload(targets, values, calldatas);

        for (uint256 i = 0; i < targets.length; i++) {
            address target = targets[i];
            uint256 value = values[i];
            bytes memory data = calldatas[i];

            // Go make our call, and if it is not successful revert with the error bubbling up
            (bool success, bytes memory returnData) = target.call{value: value}(//@audit if value is non zero this call would revert.
                data
            );

            /// revert on failure with error message if any
            require(success, string(returnData));

            emit ExecutedTransaction(target, value, data);
        }
    }

You can see that _executeProposal makes low level call to the target with value. if value is non-zero the call would revert since the contract have no ETH, since the external function is not marked payable and the contract does not implement receive payable. Another Instance: https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L266

Tools Used

Manual Review

Recommended Mitigation Steps

Add payable to the external functions calling _executeProposal or implement receive payable external so contract can recieve ETH.

Assessed type

Payable

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/WhitePaperInterestRateModel.sol#L51 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/JumpRateModel.sol#L68

Vulnerability details

Impact

The WhitePaperInterestRateModel.utilizationRate and JumpRateModel.utilizationRate function can return value above 1 and not between 0, 1e18.

Proof of Concept

In WhitePaperInterestRateModel.utilizationRate and JumpRateModel.utilizationRate function, cash and borrows and reserves values gets used to calculate utilization rate between between [0, 1e18]. reserves is currently unused but it will be used in the future. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/WhitePaperInterestRateModel.sol#L51 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/JumpRateModel.sol#L68

function utilizationRate(uint cash, uint borrows, uint reserves) public pure returns (uint) {
        // Utilization rate is 0 when there are no borrows
        if (borrows == 0) {
            return 0;
        }

        return borrows.mul(1e18).div(cash.add(borrows).sub(reserves));
    }

If Borrow value is 0, then function will return 0. but in this function the scenario where the value of reserves exceeds cash is not handled. the system does not guarantee that reserves never exceeds cash. the reserves grow automatically over time, so it might be difficult to avoid this entirely.

If reserves > cash and cash + borrows - reserves > 0, the formula for utilizationRate above gives a utilization rate above 1.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the utilization rate computation return 1 if reserves > cash.

Assessed type

Math

borrowRatePerTimestamp is not fresh and does not include interests accrued

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L236 borrowRatePerTimestamp returns the borrowing rate, but the interests accrued are not included.

function borrowRatePerTimestamp() override external view returns (uint) {
        return interestRateModel.getBorrowRate(getCashPrior(), totalBorrows, totalReserves);
    }

Impact Any smart contract relying on the value of borrowRatePerTimestamp() to perform some logic - such as decide whether to borrow from VToken based on the returned value - will be impacted.

For instance, imagine a protocol XYZ having a function in their contract that calls borrowRatePerTimestamp() and have a conditional call to mToken.borrow() if the borrowing rate is less than N. The call to borrowRatePerTimestamp() may return a value M < N, but the actual borrowing rate processed in borrow() can be P > N due to interest accrual.

Recommended Mitigation Steps

Include some interest accrual logic in borrowRatePerTimestamp() - note that it cannot be a simple call to accrueInterest as borrowRatePerBlock() is a view function.

Check the timestamp of the latest answer

chainlink states that:

During periods of low volatility, the heartbeat triggers updates to the latest answer. Some heartbeats are configured to last several hours, so your application should check the timestamp and verify that the latest answer is recent enough for your application.

Both ChainlinkCompositeOracle.getPriceAndDecimals and ChainlinkOracle.getChainlinkPrice should check that the reported answer is updated within the heartbeat or within time limits deemed acceptable:

Recommendation:

The following check could be added:

require(block.timestamp - updatedAt < PRICE_ORACLE_STALE_THRESHOLD, "STALE_THRESHOLD: Exceeded");

Moonwell Analysis Report

Moonwell is an open lending and borrowing DeFi protocol built on Base, Moonbeam, and Moonriver. It allows users to leverage their assets through the lending and borrowing process, providing them with an intuitive and seamless experience. The protocol operates on Moonbeam & Moonriver and utilizes mToken, a compound fork, in comparison to cToken. Users can earn rewards by participating in the protocol passively, either by lending mTokens or borrowing them.

Architecture Recommendation:

2.1 Token Rescue Limitation: To ensure protocol solvency, Moonwell should restrict the withdrawal of tokens that are whitelisted as collateral. Allowing this could lead to potential insolvency risks.

2.2 Accurate Reward Calculation: External View functions should accurately return rewards to facilitate third-party integration. This involves accruing interest before returning data to enhance transparency and usability.

2.3 Chainlink Best Practices: Moonwell must strictly adhere to all Chainlink best practices to avoid using stale prices. Given the highly volatile nature of the crypto market, relying on outdated prices can be dangerous.

Centralization Risks:

3.1 Admin and Governance Control: The presence of an admin controlled by governance raises concerns. This entity can access the withdrawal and rescue of stranded tokens, potentially affecting the protocol's decentralization.

3.2 Asset Price Setting: The owner's ability to set underlying asset prices as a fallback for Chainlink unavailability poses a centralization risk. Transparent price feeds should be prioritized.

3.3 Protocol Pausing: While the ability to pause the protocol serves as a safety measure, it can impact users' ability to borrow, lend, and initiate liquidations. Striking a balance between safety and protocol availability is essential for maintaining a permissionless nature.

Time Spent:

The audit process for Moonwell took a total of 3 days, with the following breakdown:

1st Day: Understanding protocol documentation. 2nd Day: Focusing on linking documentation to logic and typing reports for identified vulnerabilities, including a review of various Chainlink contracts. 3rd Day: Summarizing the audit by completing a QA report and final analysis.

Time spent:

60 hours