Moonwell - immeas's results

Lines of code

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

Vulnerability details

Impact

Wormhole governance can change signing guardian sets. If this happens between a proposal is queued and a proposal is executed. The second verification in _executeProposal will fail as the guardian set has changed between queuing and executing.

This would cause a proposal to fail to execute after the proposalDelay has passed. Causing a lengthy re-sending of the message from Timelock on source chain, then proposalDelay again.

Proof of Concept

To execute a proposal on TemporalGovernor it first needs to be queued. When queued it is checked that the message is valid against the Wormhole bridge contract:

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

File: Governance/TemporalGovernor.sol

295:    function _queueProposal(bytes memory VAA) private {
296:        /// Checks
297:
298:        // This call accepts single VAAs and headless VAAs
299:        (
300:            IWormhole.VM memory vm,
301:            bool valid,
302:            string memory reason
303:        ) = wormholeBridge.parseAndVerifyVM(VAA);
304:
305:        // Ensure VAA parsing verification succeeded.
306:        require(valid, reason);

After some more checks are done the message is queued by its wormhole hash.

Then, after proposalDelay, anyone can call executeProposal

Before the proposal is executed though, a second check against the Wormhole bridge contract is done:

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

File: Governance/TemporalGovernor.sol

344:    function _executeProposal(bytes memory VAA, bool overrideDelay) private {
345:        // This call accepts single VAAs and headless VAAs
346:        (
347:            IWormhole.VM memory vm,
348:            bool valid,
349:            string memory reason
350:        ) = wormholeBridge.parseAndVerifyVM(VAA);

The issue is that the second time the verification might fail.

During the wormhole contract check for validity there is a check that the correct guardian set has signed the message:

https://github.com/wormhole-foundation/wormhole/blob/f11299b888892d5b4abaa624737d0e5117d1bbb2/ethereum/contracts/Messages.sol#L79-L82

79:        /// @dev Checks if VM guardian set index matches the current index (unless the current set is expired).
80:        if(vm.guardianSetIndex != getCurrentGuardianSetIndex() && guardianSet.expirationTime < block.timestamp){
81:            return (false, "guardian set has expired");
82:        }

But(!), Wormhole governance can change the signers:

https://github.com/wormhole-foundation/wormhole/blob/f11299b888892d5b4abaa624737d0e5117d1bbb2/ethereum/contracts/Governance.sol#L76-L112

 76:    /**
 77:     * @dev Deploys a new `guardianSet` via Governance VAA/VM
 78:     */
 79:    function submitNewGuardianSet(bytes memory _vm) public {
            // ... parsing and verification

104:        // Trigger a time-based expiry of current guardianSet
105:        expireGuardianSet(getCurrentGuardianSetIndex());
106:
107:        // Add the new guardianSet to guardianSets
108:        storeGuardianSet(upgrade.newGuardianSet, upgrade.newGuardianSetIndex);
109:
110:        // Makes the new guardianSet effective
111:        updateGuardianSetIndex(upgrade.newGuardianSetIndex);
112:    }

Were this to happen between queueProposal and executeProposal the proposal would fail to execute.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider only check the VAAs validity against Wormhole in _executeProposal when it is fasttracked (overrideDelay==true).

However then anyone can just just take the hash from the proposal VAA and submit whatever commands. One idea to prevent this is to instead of storing the VAA vm.hash, use the hash of the complete VAA as key in queuedTransactions. Thus, the content cannot be altered.

Or, the whole VAA can be stored alongside the timestamp and the executeProposal call can be made with just the hash.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

When distributing rewards for a market, each emissionConfig is looped over and sent rewards for. disburseBorrowerRewardsInternal as an example, the same holds true for disburseSupplierRewardsInternal:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1147-L1247

File: MultiRewardDistributor/MultiRewardDistributor.sol

1147:    function disburseBorrowerRewardsInternal(
1148:        MToken _mToken,
1149:        address _borrower,
1150:        bool _sendTokens
1151:    ) internal {
1152:        MarketEmissionConfig[] storage configs = marketConfigs[
1153:            address(_mToken)
1154:        ];

             // ... mToken balance and borrow index

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

                 // ... index updates

1192:            if (_sendTokens) {
1193:                // Emit rewards for this token/pair
1194:                uint256 pendingRewards = sendReward( // <-- will trigger transfer on `emissionToken`
1195:                    payable(_borrower),
1196:                    emissionConfig.borrowerRewardsAccrued[_borrower],
1197:                    emissionConfig.config.emissionToken
1198:                );
1199:
1200:                emissionConfig.borrowerRewardsAccrued[
1201:                    _borrower
1202:                ] = pendingRewards;
1203:            }
1204:        }
1205:    }

...

1214:    function sendReward(
1215:        address payable _user,
1216:        uint256 _amount,
1217:        address _rewardToken
1218:    ) internal nonReentrant returns (uint256) {

             // ... short circuits breakers

1232:        uint256 currentTokenHoldings = token.balanceOf(address(this));
1233:
1234:        // Only transfer out if we have enough of a balance to cover it (otherwise just accrue without sending)
1235:        if (_amount > 0 && _amount <= currentTokenHoldings) {
1236:            // Ensure we use SafeERC20 to revert even if the reward token isn't ERC20 compliant
1237:            token.safeTransfer(_user, _amount); // <-- if this reverts all emissions fail
1238:            return 0;
1239:        } else {
                 // .. default return _amount
1245:            return _amount;
1246:        }
1247:    }

If one transfer reverts the whole transaction fails and no rewards will be paid out for this user. Hence if there is a malicious token that would revert on transfer it would cause no rewards to paid out. As long as there is some rewards accrued for the malicious emissionConfig. The users with already unclaimed rewards for this emissionConfig would have their rewards permanently locked.

The admin (TemporalGovernor) of MultiRewardsDistributor could update the reward speed for the token to 0 but that would just prevent further damage from being done.

Upgradeable tokens aren't unusual and hence the token might seem harmless to begin with but be upgraded to a malicious implementation that reverts on transfer.

Proof of Concept

Test in MultiRewardDistributor.t.sol, MultiRewardSupplySideDistributorUnitTest, most of the test is copied from testSupplierHappyPath with the addition of MaliciousToken:

contract MaliciousToken {
    function balanceOf(address) public pure returns(uint256) {
        return type(uint256).max;
    }

    function transfer(address, uint256) public pure {
        revert("No transfer for you");
    }
}

    function testAddMaliciousEmissionToken() public {
        uint256 startTime = 1678340000;
        vm.warp(startTime);

        MultiRewardDistributor distributor = createDistributorWithRoundValuesAndConfig(2e18, 0.5e18, 0.5e18);

        // malicious token added
        MaliciousToken token = new MaliciousToken();
        distributor._addEmissionConfig(
            mToken,
            address(this),
            address(token),
            1e18,
            1e18,
            block.timestamp + 365 days
        );

        comptroller._setRewardDistributor(distributor);

        emissionToken.allocateTo(address(distributor), 10000e18);

        vm.warp(block.timestamp + 1);

        mToken.mint(2e18);
        assertEq(MTokenInterface(mToken).totalSupply(), 2e18);

        // Wait 12345 seconds after depositing
        vm.warp(block.timestamp + 12345);

        // claim fails as the malicious token reverts on transfer
        vm.expectRevert("No transfer for you");
        comptroller.claimReward();

        // no rewards handed out
        assertEq(emissionToken.balanceOf(address(this)), 0);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a way for admin to remove an emissionConfig.

Alternatively, the reward transfer could be wrapped in a try/catch and returning _amount in catch. Be mindful to only allow a certain amount of gas to the transfer then as otherwise the same attack works with consuming all gas available.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L27 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L81-L86

Vulnerability details

Impact

guardian is mentioned as an area of concern in the docs:

Specific areas of concern include: ...

• TemporalGovernor which is the cross chain governance contract. Specific areas of concern include delays, the pause guardian, ...

guardian is a roll that has the ability to pause and unpause TemporalGovernor. In code, it uses the owner from OpenZeppelin Ownable as guardian. The issue is that Ownable::transferOwnership is not overridden. Only guardian (owner) can transfer the role.

This can be a conflict of interest if there is a falling out between governance and the guardian. If the guardian doesn't want to abstain, governance only option would be to call revokeGuardian which sets owner to address(0). This permanently removes the ability to pause the contract which can be undesirable.

Proof of Concept

Simple test in TemporalGovernorExec.t.sol:

    function testGovernanceCannotTransferGuardian() public {
        address[] memory targets = new address[](1);
        targets[0] = address(governor);
        uint256[] memory values = new uint256[](1);
        
        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSelector(Ownable.transferOwnership.selector,address(newAdmin));

        bytes memory payload = abi.encode(address(governor), targets, values, payloads);
        mockCore.setStorage(true, trustedChainid, governor.addressToBytes(admin), "reeeeeee", payload);

        governor.queueProposal("");

        vm.warp(block.timestamp + proposalDelay);

        // governance cannot transfer guardian
        vm.expectRevert(abi.encodeWithSignature("Error(string)", "Ownable: caller is not the owner"));
        governor.executeProposal("");
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider overriding transferOwnership and either limit it to only governance (msg.sender == address(this)) or both guardian and governance.

Assessed type

Governance

Lines of code

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

Vulnerability details

Description

When a guardian pauses TemporalGovernor they lose the ability to pause again. This can then be reinstated by governance using grantGuardiansPause.

However grantGuardiansPause can be called when the contract is still paused. This would break the assertions in permissionlessUnpause and togglePause: assert(!guardianPauseAllowed).

Also, TemporalGovernor inherits from OpenZeppelin Ownable. There the owner has the ability to renounceOwnership.

This could cause TemporalGovernor to end up in a bad state, since revokeGuardian call that has special logic for revoking the guardian.

There is the ability to combine these two issues to brick the contract:

1. guardian pauses the contract.
2. governance sends an instruction for grantGuardiansPause which the guardian executes (while still paused). This step disables both permissionlessUnpause and togglePause. Though the contract is still rescuable through revokeGuardian.
3. guardian calls renounceOwnership from Ownable. Which disables any possibility to execute commands on TemporalGovernor. Hence no more ability to call revokeGuardian.

Impact

The contract is bricked. Since permissionlessUnpause can't be called due to the assert(!guardianPauseAllowed). No cross chain calls an be processed because executeProposal is whenNotPaused and there is no guardian to execute fastTrackProposalExecution. Thus the TemporalGovernor will be stuck in paused state.

It is also possible for the guardian to hold the contract hostage before step 3 (renounceOwnership) as they are the only ones who can execute calls on it (through fastTrackProposalExecution).

This is scenario relies on governance sending a cross chain grantGuardiansPause while the contract is still paused and a malicious guardian which together are unlikely. Though the docs mention this as a concern:

Specific areas of concern include: ...

• 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.

Proof of Concept

Test in TemporalGovernorExec.t.sol:

    function testBrickTemporalGovernor() public {
        // 1. guardian pauses contract
        governor.togglePause();
        assertTrue(governor.paused());
        assertFalse(governor.guardianPauseAllowed());

        // 2. grantGuardianPause is called
        address[] memory targets = new address[](1);
        targets[0] = address(governor);
        uint256[] memory values = new uint256[](1);
        
        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSelector(TemporalGovernor.grantGuardiansPause.selector);

        bytes memory payload = abi.encode(address(governor), targets, values, payloads);
        mockCore.setStorage(true, trustedChainid, governor.addressToBytes(admin), "reeeeeee", payload);

        governor.fastTrackProposalExecution("");
        assertTrue(governor.guardianPauseAllowed());

        // 3. guardian revokesOwnership
        governor.renounceOwnership();

        // TemporalGovernor is bricked

        // contract is paused so no proposals can be sent
        vm.expectRevert("Pausable: paused");
        governor.executeProposal("");

        // guardian renounced so no fast track execution or togglePause
        assertEq(address(0),governor.owner());

        // permissionlessUnpause impossible because of assert
        vm.warp(block.timestamp + permissionlessUnpauseTime + 1);
        vm.expectRevert();
        governor.permissionlessUnpause();
    }

Tools Used

Manual audit

Recommended Mitigation Steps

However unlikely this is to happen, there are some simple steps that can be taken to prevent if from being possible:

Consider adding whenNotPaused to grantGuardiansPause to prevent mistakes (or possible abuse). And also overriding the calls renounceOwnership and transferOwnership. transferOwnership should be a governance call (msg.sender == address(this)) to move the guardian to a new trusted account.

Perhaps also consider if the assert(!guardianPauseAllowed) is worth just for pleasing the SMT solver.

Assessed type

DoS

Lines of code

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

Vulnerability details

Description

Wormhole cross chain communication is multicasted to all their supported chains:

Please notice that there is no destination address or chain in these functions. VAAs simply attest that "this contract on this chain said this thing." Therefore, VAAs are multicast by default and will be verified as authentic on any chain they are brought to.

The TermporalGovernor contract handles this by checking queueProposal that the intendedRecipient is itself:

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

File: core/Governance/TemporalGovernor.sol

320:        // Very important to check to make sure that the VAA we're processing is specifically designed
321:        // to be sent to this contract
322:        require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");

There is also a fastTrackProposalExecution that the guardian can perform (owner is referred ot as guardian):

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

File: core/Governance/TemporalGovernor.sol

261:    /// @notice Allow the guardian to process a VAA when the
262:    /// Temporal Governor is paused this is only for use during
263:    /// periods of emergency when the governance on moonbeam is
264:    /// compromised and we need to stop additional proposals from going through.
265:    /// @param VAA The signed Verified Action Approval to process
266:    function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
267:        _executeProposal(VAA, true); /// override timestamp checks and execute
268:    }

This will bypass the queueProposal flow and execute commands immediately.

The issue here is that there is no check in _executeProposal that the intendedRecipient is this TemporalGovernor.

Impact

governor can execute any commands communicated across Wormhole as long as they are from a trusted source.

This requires that the targets line up between chains for a governor to abuse this. However only one target must line up as the .call made does not check for contract existence. Since "unaligned" addresses between chains will most likely not exist on other chains these calls will succeeed.

Proof of Concept

Test in TemporalGovernorExec.t.sol, all of the test is copied from testProposeFailsIncorrectDestination with just the change at the end where instead of calling queueProposal, fastTrackProposalExecution is called instead, which doesn't revert, thus highligting the issue.

    function testFasttrackExecuteSucceedsIncorrectDestination() public {
        address[] memory targets = new address[](1);
        targets[0] = address(governor);

        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        TemporalGovernor.TrustedSender[]
            memory trustedSenders = new TemporalGovernor.TrustedSender[](1);

        trustedSenders[0] = ITemporalGovernor.TrustedSender({
            chainId: trustedChainid,
            addr: newAdmin
        });

        bytes[] memory payloads = new bytes[](1);

        payloads[0] = abi.encodeWithSignature( /// if issues use encode with selector
            "setTrustedSenders((uint16,address)[])",
            trustedSenders
        );

        /// wrong intendedRecipient
        bytes memory payload = abi.encode(newAdmin, targets, values, payloads);

        mockCore.setStorage(
            true,
            trustedChainid,
            governor.addressToBytes(admin),
            "reeeeeee",
            payload
        );

        // guardian can fast track execute calls intended for other contracts
        governor.fastTrackProposalExecution("");
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a check in _executeProposal for intendedRecipient same as is done in _queueProposal.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

Cross chain proposals can contain instructions to send native value. TemporalGovernor then sends this value when doing the call to the target contract:

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

File: core/Governance/TemporalGovernor.sol

400:            (bool success, bytes memory returnData) = target.call{value: value}(
401:                data
402:            );

The issue is however that TemporalGovernor has no way to receive value so it will revert since there will not be any value in the contract.

Impact

If a proposal needed requires native value, TemporalGovernor will not be able to execute it, thus impeding on the functionality of the contract. Unless extraordinary methods (SELFDESTRUCT/SENDALL) are taken to fund the contract.

Proof of Concept

Test in TemporalGovernorExec.t.sol:

    // to show it doesn't fail on transferring to `this`
    receive() external payable {} 

    function testExecuteProposalWithValue() public {
        vm.warp(1); /// queue at 0 to enable testing of message already executed path

        address[] memory targets = new address[](1);
        targets[0] = address(this);

        uint256[] memory values = new uint256[](1);
        values[0] = 1000;

        bytes[] memory payloads = new bytes[](1);
        
        /// to be unbundled by the temporal governor
        bytes memory payload = abi.encode(
            address(governor),
            targets,
            values,
            payloads
        );

        mockCore.setStorage(
            true,
            trustedChainid,
            governor.addressToBytes(admin),
            "reeeeeee",
            payload
        );
        
        governor.queueProposal("");

        bytes32 hash = keccak256(abi.encodePacked(""));
        {
            (bool executed, uint248 queueTime) = governor.queuedTransactions(
                hash
            );

            assertEq(queueTime, block.timestamp);
            assertFalse(executed);
        }

        vm.warp(block.timestamp + proposalDelay);
        
        // EvmError: OutOfFund
        vm.expectRevert();
        governor.executeProposal("");

        // revert no fallback
        vm.deal(address(this),1000);
        vm.expectRevert();
        payable(address(governor)).transfer(1000);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a receive function.

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L351-L367

Vulnerability details

Impact

mip00.sol explains the setup of the whole system. build contains the final configuration for the deployment.

There each MToken is unpaused and an initial mint is done to prevent exchange rate manipulation.

The issue is that each token uses the same initialMintAmount. In the comments ETH and USDC are mentioned as tokens. Since these have different decimals the real value used for minting will be very different.

initialMintAmount is 1 ether in the base contract Configs.sol. Since it is unlikely that the deploying contract will have 1e18 USDC ($1 trillion) the build part of the setup and configuration will likely fail. Even if this isn't the actual initial mint amount there is no number that would fit both 6 decimal USDC and 18 decimal ETH at the same time.

Proof of Concept

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L334-L379

File: test/proposals/mips/mip00.sol

                // each token config is looped over
334:            for (uint256 i = 0; i < cTokenConfigs.length; i++) {
335:                Configs.CTokenConfiguration memory config = cTokenConfigs[i];
336:
337:                address cTokenAddress = addresses.getAddress(
338:                    config.addressesString
339:                );

                    // ... unpause mint

351:                /// Approvals
352:                _pushCrossChainAction(
353:                    config.tokenAddress,
354:                    abi.encodeWithSignature(
355:                        "approve(address,uint256)",
356:                        cTokenAddress,
357:                        initialMintAmount // <-- same `initialMintAmount` for all tokens
358:                    ),
359:                    "Approve underlying token to be spent by market"
360:                );
361:
362:                /// Initialize markets
363:                _pushCrossChainAction(
364:                    cTokenAddress,
365:                    abi.encodeWithSignature("mint(uint256)", initialMintAmount), // <-- same `initialMintAmount` for all tokens
366:                    "Initialize token market to prevent exploit"
367:                );

                    // ... set collateral factor
379:            }

initialMintAmount can be found to be 1 ether in base contract Configs.sol:

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/Configs.sol#L54-L55

File: test/proposals/Configs.sol

54:    /// @notice initial mToken mint amount
55:    uint256 public constant initialMintAmount = 1 ether;

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a field in the configuration for initialMintAmount for each token.

Assessed type

ERC20

QA Report

Summary

Low

L-01 guardian can fast track every proposal

In TemporalGovernor the guardian has a special permission that they can fastTrackProposalExecution which will bypass the normal queueProposal flow:

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

File: core/Governance/TemporalGovernor.sol

261:    /// @notice Allow the guardian to process a VAA when the
262:    /// Temporal Governor is paused this is only for use during
263:    /// periods of emergency when the governance on moonbeam is
264:    /// compromised and we need to stop additional proposals from going through.
265:    /// @param VAA The signed Verified Action Approval to process
266:    function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
267:        _executeProposal(VAA, true); /// override timestamp checks and execute
268:    }

This as the comment suggest should only be done in emergencies.

However there is nothing indicating that the VAA in question is supposed to be fast tracked. Hence a guardian can always fast track any VAAs that they please, bypassing the queueProposal functionality.

Recommendation

Consider adding another parameter to the payload indicating if the VAA is intended to be fast tracked or not.

L-02 TemporalGovernor can get the same address on different chains

In mip00.sol the deploy and configuration proceedure for the TemporalGovernor is detailed:

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L103-L109

File: test/proposals/mips/mip00.sol

103:            /// this will be the governor for all the contracts
104:            TemporalGovernor governor = new TemporalGovernor(
105:                addresses.getAddress("WORMHOLE_CORE"),
106:                proposalDelay,
107:                permissionlessUnpauseTime,
108:                trustedSenders
109:            );

new will invoke the CREATE opcode. This only relies on the msg.sender and nonce. Since the deploy call executes a certain amount of calls when it executes the deploy of TemporalGovernor it will have certain nonce. Hence the address of TemporalGovernor will be dependent on the account that executes it. If this is run by a new account the address of TemporalGovernor can be the same if it is executed by the same new account on another chain.

This is important because if two TemporalGovernor contracts on different chains gets the same address they can execute each others proposals:

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

File: core/Governance/TemporalGovernor.sol

320:        // Very important to check to make sure that the VAA we're processing is specifically designed
321:        // to be sent to this contract
322:        require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");

Recommendation

Pay attention to which accounts and how the execution of the deploy script is done. Perhaps use CREATE2 with a random salt to deploy TemporalGovernor to guarantee it gets a different address on each chain.

L-03 TemporalGovernor::grantGuardiansPause can be called after guardian is revoked

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

File: Governance/TemporalGovernor.sol

187:    /// @notice grant the guardians the pause ability
188:    function grantGuardiansPause() external {
189:        require(
190:            msg.sender == address(this),
191:            "TemporalGovernor: Only this contract can update grant guardian pause"
192:        );
193:
194:        guardianPauseAllowed = true;
195:        lastPauseTime = 0;
196:
197:        emit GuardianPauseGranted(block.timestamp);
198:    }

This doesn't do any thing bad other than set guardianPauseAllowed=true, which is set to false in revokeGuardian.

Recommendation

Consider only allowing calls to grantGuardiansPause when guardian != address(0).

L-04 No cap on how many emissionConfigs there can be for a market

Too many emissionConfigs could lead to out of gas errors when updating reward indexes. Since every operation on an mToken triggers this the whole market would be DoSed if too many emissionConfigs would be added.

Recommendation

Consider adding a way to remove an emissionConfig

L-05 Neither _setMarketBorrowCaps or _setMarketSupplyCaps checks if market is listed

When adding a cap on borrow/supply admin or borrow/supplyCapGuardian can call [_setMarketBorrowCaps])(https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L807-L819) or _setMarketSupplyCaps respectively.

However in none of the there is any check that the market (MToken) for which they add the supply/borrow cap is actually listed. Hence admin or guardian could by mistake send the wrong address and then think that they configured the cap for that market when they didn't.

Recommendation

Add a check that the MToken is listed when setting borrow/supply caps.

L-06 Lack of tests for supplyCap

M̀oonwell introduces a new feature to the Comptroller: supplyCap per market. This limits how much can be minted to that market. There is however no tests for this new feature. Test guarantee that the feature works as expected and will continue to work as expected when doing changes to the code. As such they are a core guarantee for code and protocol safety.

Consider adding a few tests for the supplyCap feature

Suggestions

S-01 ChainlinkCompositeOracle::getDerivedPriceThreeOracles is very specialized

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

File: Oracles/ChainlinkCompositeOracle.sol

157:        return
158:            ((firstPrice * secondPrice * thirdPrice) / scalingFactor)
159:                .toUint256();

Here the three prices for the oracles are multiplied together. This requires a setup of prices like this: First one is stated to be ETH/USD

so there would have to be oracles:

ETH/USD, A/ETH, B/A to get the price B/USD

This is very limited which chainlink prices support "chaining" like this, the one used in integration test seems to one of the few:

ETH/USD, stETH/ETH, wstETH/stETH on Arbitrum.

Recommendations

Consider adding the ability to either multiply or divide by the last one, as that would give greater flexibility in which feeds can be used.

Refactor

R-01 MultiRewardDistributor::calculateNewIndex parameter _currentTimestamp is confusing

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

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

File: MultiRewardDistributor/MultiRewardDistributor.sol

906:     * @param _currentTimestamp The current index timestamp

914:        uint32 _currentTimestamp,

_currentTimestamp implies now when it is however the last index timestamp. Consider renaming it to lastIndexTimestamp.

R-02 Unnecessary _amount> 0 check

In sendReward, the function sort circuit if _amount to be sent is 0, later however it is checked that this amount is >0 which there is no way it cannot be:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1219-L1222

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

File: MultiRewardDistributor/MultiRewardDistributor.sol

1219:        // Short circuit if we don't have anything to send out
1220:        if (_amount == 0) {
1221:            return _amount;
1222:        }

			 // @audit due to check above there is no way _amount cannot be >0
1235:        if (_amount > 0 && _amount <= currentTokenHoldings) {

Consider removing the _amount > 0 check since it is always true.

R-03 MultiRewardDistributor: IndexUpdate struct is unnecessary

IndexUpdate is used to return the result of calculateNewIndex.

However in calculateNewIndex the same value for IndexUpdate.newTimestamp is always used, block.timestamp:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L949-L953

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

949:            return
950:                IndexUpdate({
951:                    newIndex: _currentIndex,
952:                    newTimestamp: blockTimestamp
953:                });

967:        return IndexUpdate({newIndex: newIndex, newTimestamp: blockTimestamp});

Hence, calculateNewIndex could simply return newIndex (uin224). Then in updateMarketSupplyIndexInternal and updateMarketBorrowIndexInternal the supply/borrowGlobalTimestamp can be set directly to block.timestamp.

R-04 parameter _mTokenData for MultiRewardDistributor::calculateBorrowRewardsForUser is unnecessary

The struct MTokenData has two fields: mTokenBalance and borrowBalanceStored. However only one of them is used in calculateBorrowRewardsForUser.

Therefore it is confusing that the whole struct is passed to the function as that implies that both fields would be used.

Consider just passing borrowBalanceStored directly instead of the struct.

Non critical / Informational

NC-01 wrong grammatical number in grantGuardiansPause

grantGuardiansPause grants guardian pause. Since there is only one guardian the name should be singular not plural: grantGuardianPause.

NC-02 erroneous docs

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

Guardian Pause: The guardian has the ability to pause the contract temporarily. When the guardian pauses the contract, all proposal executions are halted. The contract can only be unpaused by a governance proposal after a specified time delay.

This is not true. A governance proposal cannot unpause the TemporalGovernor. Unpause can only be performed by governor through togglePause or by the permissionlessUnpause after a delay. The only way governance could unpause is by calling revokeGuardian which is final. This would require the guardian to execute it though since only guardian is allowed to execute commands while the contract is paused.

NC-03 weird word describing the future

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L788-L792

File: MultiRewardDistributor/MultiRewardDistributor.sol

788:        // Must be older than our existing end time AND the current block
789:        require(
790:            _newEndTime > currentEndTime,
791:            "_newEndTime MUST be > currentEndTime"
792:        );

older here is confusing. Either say Must be further in the future or simply larger

NC-04 forgotten cToken references

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

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

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

File: MultiRewardDistributor/MultiRewardDistributor.sol

842:        // Calculate change in the cumulative sum of the reward per cToken accrued

843:        // Calculate reward accrued: cTokenAmount * accruedPerCToken

844:        // Calculate change in the cumulative sum of the reward per cToken accrued

NC-05 incorrect comments

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L835-L840

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L874-L879

File: MultiRewardDistributor/MultiRewardDistributor.sol

835:        // If our user's index isn't set yet, set to the current global supply index
836:        if (
837:            userSupplyIndex == 0 && _globalSupplyIndex >= initialIndexConstant
838:        ) {
839:            userSupplyIndex = initialIndexConstant; //_globalSupplyIndex;
840:        }

...

874:        // If our user's index isn't set yet, set to the current global borrow index
875:        if (
876:            userBorrowIndex == 0 && _globalBorrowIndex >= initialIndexConstant
877:        ) {
878:            userBorrowIndex = initialIndexConstant; //userBorrowIndex = _globalBorrowIndex;
879:        }

If the index is not yet set it's set to initialIndexConstant not the global index as the comment suggests

NC-06 comment slash off by one

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L984-L988

   /** <-- slash only 3 whitespaces in
     * @notice Call out to the reward distributor to update its borrow index and this user's index too
     * @param mToken The market to synchronize indexes for
     * @param borrower The borrower to whom rewards are going
     */

The slash is only 3 not 4 whitespaces in.