code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/CToken.sol#L781 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/CToken.sol#L905

Vulnerability details

Impact

Ctoken is vulnerable to re-entrancy because it doesn't implement CEI (check-effect-interaction) pattern for borrow functionality. borrowFresh() and redeemFresh() function are vulnerable to re-entrancy attack.

BorrowFresh/RedeemFresh is vulnerable to re-entrancy attack. (https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/CToken.sol#L781 - https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/CToken.sol#L905)

As an example You decided to support ERC777 Token. On the cream finance hack, you can clearly see an attack vector. You don't need deploy malicious token. You just deploy ERC777 token.

On the CreamFinance hack, The AMP token contract implements ERC777, which has the _callPostTransferHooks hook that triggers tokensReceived() function that was implemented by the recipient. The reentrancy opportunity related to ERC-777-style transfer hooks allowed the exploiter to nest a second borrow() function inside the token transfer() before the initial borrow() was updated.

Impact of the vulnerability right now is 0 if there aren't any ERC777 (or any other ERC20 that does the callback) added to the protocol. But in the future it may be possible that such token could be added. Cream finance was hacked same way Cream finance was exploited using the same vulnerability when they integrated AMP ERC777 token (https://medium.com/cream-finance/c-r-e-a-m-finance-post-mortem-amp-exploit-6ceb20a630c5)

Proof of Concept

The following steps can be seen from the PoC.

Borrow BadToken for CToken from CBadToken.
During the transfer of bad tokens to the borrower, we do a callback to the malicious contract. In the malicious contract we transfer badtokens we just got to the second contract.
We do the same with the remaining CToken. In the second malicious contract we redeem Token and mint CBadToken for the BadTokens in step 2. We borrow Ctoken for CBadTokens.
Due to lack of checks-effects-interaction pattern, totalBorrows is updated after the hack is completed. totalBorrows is used in - exchangeRateStoredInternal for calculating the exchange rate which is used for calculating the redeemAmount.

Additional Info :

How did the CREAM attack work?

As we already saw, the order of the CREAM borrow function was vulnerable to reentrancy. Furthermore, a coin that allowed an attackerto execute code during a transfer was added to the list of tokens supported by CREAM.

However, the CREAM lending pool contracts did have reentrancy locks on their contract functions. How did the contracts get hacked when they had protection here?

The CREAM lending system is made up of multiple contracts with one "CToken" pool contract for each asset supported. So CREAM has a lending contract for ETH, as it does for USDC, as it does for AMP, etc. Each one of these individual lending contracts has its own separate lock for protecting that individual contract against reentrancy.

However, the system as a whole is not protected because an attacker could be inside different lending pool contracts simultaneously.

In each round of this attack, the attacker put up one and a half million dollars of collateral, then borrowed AMP. During the borrow function for in the AMP pool contract, the AMP coin called the attackers code, allowing the attacker to start a second borrow of ETH. Because the AMP borrow had not written any record of the AMP borrow to storage yet, when the ETH borrow in the ETH pool contract checked with all pool contracts to see how much the attacker had already borrowed, it saw that the attacker had no debts and lots of collateral, and thus allowed a duplicate ETH borrow.

Tools Used

Code Review

Recommended Mitigation Steps

Follow Check Effect Interaction Pattern

  /////////////////////////
        // EFFECTS & INTERACTIONS
        // (No safe failures beyond this point)

        /* We write the previously calculated values into storage */
        accountBorrows[borrower].principal = vars.accountBorrowsNew;
        accountBorrows[borrower].interestIndex = borrowIndex;
        totalBorrows = vars.totalBorrowsNew;

        /*
         * We invoke doTransferOut for the borrower and the borrowAmount.
         *  Note: The pToken must handle variations between ERC-20 and ETH underlying.
         *  On success, the pToken borrowAmount less of cash.
         *  doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
         */
        doTransferOut(borrower, borrowAmount);

Reference : https://twitter.com/CreamdotFinance/status/1432249771750686721

https://github.com/OriginProtocol/security/blob/master/incidents/2021-08-31-Cream.md

0xdramaone commented 2 years ago

Disagree that this is of concern with the global reentrancy lock from Rari’s compound implementation. Believe this to be low risk, if any.

That said, we can change to checks-effects-interactions pattern in these functions for good measure.

jack-the-pug commented 2 years ago

Not a valid issue as the nonReentrant modifiers already exit at a higher level.

code-423n4 / 2022-04-dualityfocus-findings