Moonwell - 0xkazim's results

Lines of code

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

Vulnerability details

Impact

the chainlinkOracle.sol contract specially the getChainlinkPrice function using the aggregator v2 and v3 to get/call the latestRoundData. the function should check for the min and max amount return to prevent some case happen, something like this:

https://solodit.xyz/issues/missing-checks-for-chainlink-oracle-spearbit-connext-pdf https://solodit.xyz/issues/m-16-chainlinkadapteroracle-will-return-the-wrong-price-for-asset-if-underlying-aggregator-hits-minanswer-sherlock-blueberry-blueberry-git

if case like luna happen then the oracle will return the minimum price and not the crashed price.

Proof of Concept

the function getChainlinkPrice:

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");

        // Chainlink USD-denominated feeds store answers at 8 decimals
        uint256 decimalDelta = uint256(18).sub(feed.decimals());
        // Ensure that we don't multiply the result by 0
        if (decimalDelta > 0) {
            return uint256(answer).mul(10 ** decimalDelta);
        } else {
            return uint256(answer);
        }
    }

the function did not check for the min and max price.

Tools Used

manual review

Recommended Mitigation Steps

some check like this can be added to avoid returning of the min price or the max price in case of the price crashes.

          require(answer < _maxPrice, "Upper price bound breached");
          require(answer > _minPrice, "Lower price bound breached");

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L241-L259 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L274-L290 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L202-L221

Vulnerability details

Impact

the TemporalGovernor implemented in a way that if the guardians pause the system then the system will be paused to period of time and the system will remove the guradians approve till the system reApprove them again and going to the unpause mood, the system really focus on to unpause the contract only if the specifc time is passed in the permissionlessUnpause function. but any guardian can unpause the system even before the required time is passed. more details in POC

Proof of Concept

the guardians can pause the system by calling the togglePause:

    function togglePause() external onlyOwner {
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

and if the system is paused then the guardians should only be allowed to unpause the system only if period of time has passed in the permissionlessUnpause:

function permissionlessUnpause() external whenPaused {
        /// lastPauseTime cannot be equal to 0 at this point because
        /// block.timstamp on a real chain will always be gt 0 and
        /// toggle pause will set lastPauseTime to block.timestamp
        /// which means if the contract is paused on a live network,
        /// its lastPauseTime cannot be 0
        //@audit can be called if required time is passed
        require(
            lastPauseTime + permissionlessUnpauseTime <= block.timestamp,
            "TemporalGovernor: not past pause window"
        );

        lastPauseTime = 0;
        _unpause();

        assert(!guardianPauseAllowed); /// this should never revert, statement for SMT solving

        emit PermissionlessUnpaused(block.timestamp);
    }

but this won't be true because the guardians(the 19 guardians) can unpause the systme by calling one of these two functionstogglePause and revokeGuardian which allow to unpause the system without need of passing specific time passed.

the togglePause will unpause the system if the system was paused already without need to wait for specific time to passed:

 function togglePause() external onlyOwner {
    //@audit can be called when the contract is paused and set it to the unpause mood
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

same thing for the revokeGuardian:

function revokeGuardian() external {
        //@audit bad guardian can unpause the systme even if the time not passed !

        address oldGuardian = owner();
        require(
            msg.sender == oldGuardian || msg.sender == address(this),
            "TemporalGovernor: cannot revoke guardian"
        );

        _transferOwnership(address(0));
        guardianPauseAllowed = false;
        lastPauseTime = 0;

        if (paused()) {
            _unpause();
        }

        emit GuardianRevoked(oldGuardian);
    }

the protocol trust in the guardians but this not recommended and the protocol should think about this case and how to prevent it in case the guardians get hacked or act malicousliy. even it mention in the docs that If Moonbeam governance is compromised, the contract will be paused until governance control is restored. in this case the system should be care about prevent any mistakliy unpause the system:

https://github.com/code-423n4/2023-07-moonwell/blob/main/docs/TEMPORALGOVERNOR.md

the 19 guardians by wormhole: https://wormholecrypto.medium.com/wormhole-guardians-everstake-83afb735cddd

Tools Used

manual review

Recommended Mitigation Steps

recommend to add the same check in the permissionlessUnpause function to avoid unpause the system in bad time and bad case even by mistake or by an malicious guardians.

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

the function getPrice may underflow if the mToken decimals was more than 18. this can happen because mToken can be any token with decimals more than 18 and in this case the function getPrice() will underflow and revert.

Proof of Concept

the function getPrice:

    function getPrice(MToken mToken) internal view returns (uint256 price) {
        EIP20Interface token = EIP20Interface(
            MErc20(address(mToken)).underlying()
        );

        if (prices[address(token)] != 0) {
            price = prices[address(token)];
        } else {
            //add return here
            price = getChainlinkPrice(getFeed(token.symbol()));
        }
        /**
        @audit underflow may happen, some tokens have more than 18 tokens
        */
        uint256 decimalDelta = uint256(18).sub(uint256(token.decimals()));
        // Ensure that we don't multiply the result by 0
        if (decimalDelta > 0) {
            return price.mul(10 ** decimalDelta);
        } else {
            return price;
        }
    }

the function will get the decimal delta using the token decimals which can be more than 18 and cause underflow.

Tools Used

manual review

Recommended Mitigation Steps

recommend to add check for token.decimals <= 18 to avoid underflow

Assessed type

Decimal