search

Cyfrin / 2023-07-beedle

21 stars 20 forks source link

Stealing any loan opening for auction through others' lending pool #1810

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Stealing any loan opening for auction through others' lending pool

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L489

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L518

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L304

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L311-L312

Summary

An attacker can steal any loan opening for auction for free by executing the Lender::buyLoan() and inputting anyone else's poolId as an argument.

Vulnerability Details

Root cause: the buyLoan() lacks verification that a caller (msg.sender) must be the new pool's lender.

Therefore, an attacker (which can be anyone, including a current pool's lender) can execute the buyLoan() to force anyone else's lending pool to pay the loan's total debt for the attacker.

Subsequently, the forced pool has to pay for the debt but the attacker will become a new lender for free.

    function buyLoan(uint256 loanId, bytes32 poolId) public {
        ...

        // if they do have a big enough pool then transfer from their pool
@>      _updatePoolBalance(poolId, pools[poolId].poolBalance - totalDebt); //@audit the forced pool has to pay for the debt
        pools[poolId].outstandingLoans += totalDebt;

        ...

        // update the loan with the new info
@>      loans[loanId].lender = msg.sender; //@audit the attacker will become a new lender
        loans[loanId].interestRate = pools[poolId].interestRate;
        loans[loanId].startTimestamp = block.timestamp;
        loans[loanId].auctionStartTimestamp = type(uint256).max;
        loans[loanId].debt = totalDebt;

        ...
    }

Once a borrower repays the loan, the attacker address will be used to compute the poolId. The computed poolId will point to the attacker's pool. This way, the attacker can steal the loan principal (loan.debt) and interest (lenderInterest).

    function repay(uint256[] calldata loanIds) public {
        for (uint256 i = 0; i < loanIds.length; i++) {
            ...

            bytes32 poolId = getPoolId(
@>              loan.lender,
                loan.loanToken,
                loan.collateralToken
            );

            // update the pool balance
            _updatePoolBalance(
@>              poolId,
@>              pools[poolId].poolBalance + loan.debt + lenderInterest
            );

            ...
        }
    }

Impact

An attacker can steal any loan opening for auction for free by executing the buyLoan() and inputting anyone else's poolId as an argument. This vulnerability is considered a high-risk issue.

Tools Used

Manual Review

Recommendations

I recommend verifying that a caller (msg.sender) of the buyLoan() must be the new pool's lender, as shown below.

    function buyLoan(uint256 loanId, bytes32 poolId) public {
        ...

+       if (msg.sender != pools[poolId].lender) revert Unauthorized();

        ...

        // if they do have a big enough pool then transfer from their pool
        _updatePoolBalance(poolId, pools[poolId].poolBalance - totalDebt);
        pools[poolId].outstandingLoans += totalDebt;

        ...

        // update the loan with the new info
        loans[loanId].lender = msg.sender;
        loans[loanId].interestRate = pools[poolId].interestRate;
        loans[loanId].startTimestamp = block.timestamp;
        loans[loanId].auctionStartTimestamp = type(uint256).max;
        loans[loanId].debt = totalDebt;

        ...
    }
PatrickAlphaC commented 1 year ago

Also see https://github.com/Cyfrin/2023-07-beedle/issues/597

kosedogus commented 1 year ago

This issue and all of its duplicates are same as H6: #1166 and its duplicates . Actually so many of issues that are selected for both this (H8) and (H6) are contains both labels. Both issues are talking about stealing loan's from other pools because there is no access control to check if msg.sender is pool's lender. I can provide more details if necessary but I believe it is not hard to see when compared side to side.

kosedogus commented 1 year ago

H9: #2008 and all of its duplicates are also same as this issue (hence with H6). Although the impact is different the root cause, and solution to issues are same. Because CodeHawks does not have proper documentation regarding duplicate rules yet, I would like to provide rules from other contest platforms documentation. Sherlock:

Issues identifying a core vulnerability can be considered duplicates. Scenario 1: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attacks paths:

  • B -> high severity path
  • C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.

Code4rena

All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path. However, any submissions which do not identify or effectively rationalize the top identified severity case may be judged as “partial credit” and may have their shares in that finding’s pie divided by 2 or 4 at judge’s sole discretion (e.g. 50% or 25% of the shares of a satisfactory submission in the duplicate set).

Jnrlouis commented 1 year ago

Adding to what @kosedogus said. I believe this is a duplicate issue at the root, and the impact stated by this auditor is not exactly right. The auditor stated: Once a borrower repays the loan, the attacker address will be used to compute the poolId. The computed poolId will point to the attacker's pool. This way, the attacker can steal the loan principal (loan.debt) and interest (lenderInterest).

But this assumption is wrong, the borrower wouldn't be able to repay the loan, hence the attacker cannot steal the loan, what would happen is a DOS.

I wrote a POC for this: https://github.com/Cyfrin/2023-07-beedle/issues/1009#issue-1839716687

The reason the borrower wouldn't be able to repay the loan is from the repay function, the poolId is recalculated as shown using the attacker's address as the loan.lender, this would generate an invalid pool Id, and it would revert due to underflow at this line.

asendz commented 1 year ago

Agree with @kosedogus

Being able to buy loans on behalf of other lenders pools opens up a lot of different variations and possible attack vectors. I don't see a reason why every variation should be considered a separate issue when just fixing the root cause fixes all of them.

This just leads to finding 1 issue and describing it in 5 different ways about how something can go wrong and submitting them as separate.

PatrickAlphaC commented 1 year ago

Thanks for all the context here. I think in this case, the attack paths are important, and I lean more towards sherlock's methodology. We will have to update the docs for CodeHawks takes on these. Might have to do some voting.

For this context, I thought the attack paths were different enough to include them as separate findings, which would be similar to Sherlock's "partial credit". Hence why I gave many of these many tags of similar issues.

For the moment, I think these are different findings:

  1. it doesn't mention that repay gets bricked, so we won't have it be a dup of the bricked finding.
  2. it does mention the attacker becoming loan.lender, and having their own pool, so the repay will work and go to the attacker's pool. This is one of the major common factors for all findings in the label finding-buyLoan-steal-collateral-pool-drain - that the attacker can make the repay work by having their own pool

Because issues can have multiple tags for v0.1 of CodeHawks, I think this is a partial duplicate of https://github.com/Cyfrin/2023-07-beedle/issues/1166.

We should update the judging for paths like this moving forward. Thank you all for your help on this. At the end of the day, the protocol is going to get the information they need from these findings to fix, and we have opted for the max(oldReward, highReward) so that there is some lee-way in the prize pools for this first contest.

Any of you down to make a PR to the codehawks docs for what you think make sense in this scenario?

https://github.com/Cyfrin/codehawks

Thank you