EVM Contracts - Moonwell

The ChainlinkOracle contract uses Chainlink’s deprecated API latestAnswer(). Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs. This method will return the last value, but it is possible to check if the data is fresh.

Code Location

ChainlinkOracle.sol

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

SOLVED: This issue was solved by implementing better ChainLink Oracle API call (latestRoundData()).

Commit ID: e23657c5fbeb12c7393fa49da6f350dc0bd5114e && 762cdc4cd9a8d09f29765f9e143b25af0ebe9720.

Eth sent to Timelock will be locked in current implementation.

Code Location

GovernorAlpha.sol

function execute(uint proposalId) external {
        require(state(proposalId) == ProposalState.Queued, "GovernorAlpha::execute: proposal can only be executed if it is queued");
        Proposal storage proposal = proposals[proposalId];
        proposal.executed = true;
        for (uint i = 0; i < proposal.targets.length; i++) {
            timelock.executeTransaction(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
        }
        emit ProposalExecuted(proposalId);
    }

• Set up the governance contracts (GovernanceAlpha, Timelock).
• Send eth to timelock contract.
• Set up a proposal to send 0.1 eth out. Code snippet in ether.js below. proxy refers to GovernorAlpha.

    await proxy.propose(
      [signers[3].address],
      [ethers.utils.parseEther("0.1")],
      [""],
      [ethers.BigNumber.from(0)],
      "Send funds to 3rd signer"
    );

• Vote and have the proposal succeed.
• Execute the proposal, the proposal number here is arbitrary.

await proxy.execute(2);  // this fails
await proxy.execute(2, {value: ethers.utils.parseEther("0.1")})  // this would work

• 0.1 eth will be sent out, but it is sent from the msg.sender not from the timelock contract.

SOLVED: This issue was solved by removing payable keyword and call.value() method from the execute() function on GovernorAlpha.sol contract.

Commit ID: e23657c5fbeb12c7393fa49da6f350dc0bd5114e && 762cdc4cd9a8d09f29765f9e143b25af0ebe9720.

If votes variable is equal to zero on GovernorAlpha.sol:_castVote() method contract should short circuit itself to consume less gas. The following lines will be executed even if votes amount is zero.

GovernorAlpha.sol

if (support) {
            proposal.forVotes = add256(proposal.forVotes, votes);
        } else {
            proposal.againstVotes = add256(proposal.againstVotes, votes);
        }

        receipt.hasVoted = true;
        receipt.support = support;
        receipt.votes = votes;

        emit VoteCast(voter, proposalId, support, votes);
    }

Basically, the contract will call more functions such as add256() even it is not necessary.

The same issue also exists on Well.sol:transferFrom() function. If rawAmount parameter is equal to zero, the contract should short-circuit itself to prevent gas consume. The following lines will be executed even if rawAmount is equal to zero.

Well.sol

if (spender != src && spenderAllowance != uint96(-1)) {
            uint96 newAllowance = sub96(spenderAllowance, amount, "Well::transferFrom: transfer amount exceeds spender allowance");
            allowances[src][spender] = newAllowance;

            emit Approval(src, spender, newAllowance);
        }

        _transferTokens(src, dst, amount);
        return true;

Code Location

Vulnerable Functions

GovernorAlpha.sol:_castVote(address voter, uint proposalId, bool support)
Well.sol:transferFrom(address src, address dst, uint rawAmount)

NOT APPLICABLE: This issue was marked as NOT APPLICABLE since the recommendation does not fit to intended behavior of Compound Protocol. Furthermore, Moonwell Team stay as close to the original contracts as possible, even if they are not completely optimal concerning gas efficiency, so that improvements to the original contracts may be adopted without significant refactoring, and the community can have better certainty that they function similarly to other contracts with the same code.

The cancel(uint proposalId) The cancel function is used to cancel the proposals. There is a check on the contract to do not cancel executed proposals. If the state of a proposal is not QUEUED yet, the contract will revert to cancel function. However, it will consume gas to achieve this. There is a missing control on the contract to only cancel QUEUED proposals.

Code Location

GovernorAlpha.sol

function cancel(uint proposalId) public {
        ProposalState state = state(proposalId);
        require(state != ProposalState.Executed, "GovernorAlpha::cancel: cannot cancel executed proposal");

        Proposal storage proposal = proposals[proposalId];
        require(msg.sender == guardian || well.getPriorVotes(proposal.proposer, sub256(block.number, 1)) < proposalThreshold(), "GovernorAlpha::cancel: proposer above threshold");

        proposal.canceled = true;
        for (uint i = 0; i < proposal.targets.length; i++) {
            timelock.cancelTransaction(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
        }

        emit ProposalCanceled(proposalId);
    }

NOT APPLICABLE: This issue was marked as NOT APPLICABLE since the recommendation does not fit to intended behavior of Compound Protocol.

“A proposal is eligible to be cancelled at any time before its execution, including while queued in the Timelock, using this function.”

Moonwell-Core contracts have multiple input fields on their both public and private functions. Some of these inputs are required as address variable. During the test, it has seen all of these inputs are not protected against using the address(0) as the target address. It is not recommended to use zero address as target addresses on the contracts.

Code Location

Missing Zero Address Checks

ChainlinkOracle.setAdmin(address).newAdmin (contracts/Chainlink/ChainlinkOracle.sol#88)
Comptroller._setBorrowCapGuardian(address).newBorrowCapGuardian (contracts/Comptroller.sol#968)
Comptroller._setPauseGuardian(address).newPauseGuardian (contracts/Comptroller.sol#986)
Comptroller.setWellAddress(address).newWellAddress (contracts/Comptroller.sol#1351)
MErc20.initialize(address,ComptrollerInterface,InterestRateModel,uint256,string,string,uint8).underlying_ (contracts/MErc20.sol#21)
MToken._setPendingAdmin(address).newPendingAdmin (contracts/MToken.sol#1144)
MErc20Delegator.constructor(address,ComptrollerInterface,InterestRateModel,uint256,string,string,uint8,address,address,bytes).admin_ (contracts/MErc20Delegator.sol#31)
MErc20Delegator._setImplementation(address,bool,bytes).implementation_ (contracts/MErc20Delegator.sol#60)
MErc20Immutable.constructor(address,ComptrollerInterface,InterestRateModel,uint256,string,string,uint8,address).admin_ (contracts/MErc20Immutable.sol#29)
MGlimmer.constructor(ComptrollerInterface,InterestRateModel,uint256,string,string,uint8,address).admin_ (contracts/MGlimmer.sol#27)
Reservoir.constructor(uint256,EIP20Interface,address).target_ (contracts/Reservoir.sol#32)
Timelock.constructor(address,uint256).admin_ (contracts/Timelock.sol#26)
Timelock.executeTransaction(address,uint256,string,bytes,uint256).target (contracts/Timelock.sol#81)
Unitroller._setPendingImplementation(address).newPendingImplementation (contracts/Unitroller.sol#38)
Unitroller._setPendingAdmin(address).newPendingAdmin (contracts/Unitroller.sol#85)

RISK ACCEPTED: The Moonwell Team accepts the risk of this finding. Except the ChainlinkOracle contract, all the contracts mentioned require the new admin key to execute the _acceptPendingAdmin function, which protects against accidental attempts to set the admin or guardian to the zero address. It was decided not to make any changes.

During the test, it was determined that a variable on the contract was not used for any purpose, although it was defined on the contract. This situation does not pose any risk in terms of security. But it is important for the readability and applicability of the code.

The baseRatePerYear parameter of updateJumpRateModel function on DAIInterestRateModelV3.sol contract is unused on that function.

Code Location

DAIInterestRateModelV3.sol

function updateJumpRateModel(uint baseRatePerYear, uint gapPerYear, uint jumpMultiplierPerYear, uint kink_) external {
        require(msg.sender == owner, "only the owner may call this function.");
        gapPerTimestamp = gapPerYear / timestampsPerYear;
        updateJumpRateModelInternal(0, 0, jumpMultiplierPerYear, kink_);
        poke();
    }

SOLVED: This issue has was solved by the Moonwell Team. The DAIInterestRateModel.sol contract is not used by the Moonwell community, so this contract has been removed from the repository.