sherlock-admin2 commented 7 months ago

0x3b

high

Borrowers can surpass `liquidityThresholdPercent` and borrow to near 100% of the principal

Summary

Borrowers can borrow up to 100% of the principal due to getPrincipalAmountAvailableToBorrow relying on deposited principal. This will in tern lock LPs and prevent them from withdrawing.

Vulnerability Detail

acceptFundsForAcceptBid includes a check that prevents borrowers from borrowing more than the liquidityThresholdPercent of the principal. This measure is intended to ensure safe withdrawals for LPs even when demand for borrowing exceeds the available principal. If this threshold is reached, no more assets can be borrowed, though LPs are still free to deposit and withdraw.

However, this check relies on getPoolTotalEstimatedValue, which includes totalPrincipalTokensCommitted - the total tokens deposited.

    function getPrincipalAmountAvailableToBorrow() public view returns (uint256) {     
            return (uint256(getPoolTotalEstimatedValue())).percent(liquidityThresholdPercent) -
            getTotalPrincipalTokensOutstandingInActiveLoans();
    }

With this in mind, if liquidityThresholdPercent is reached, borrowers can simply deposit some collateral to increase the getPrincipalAmountAvailableToBorrow ratio, borrow the newly available collateral, and withdraw their original stake.

Example:

liquidityThresholdPercent is 80%, expected to be sufficient to ensure normal LP withdrawals.
Pool value (getPoolTotalEstimatedValue) is $100k.
Assets borrowed are $80k.
getPrincipalAmountAvailableToBorrow returns (100k * 80%) - 80k = 0 -> no borrowing allowed.

Steps:

Alice deposits $25k principal (flash loans can be used, but are not required).
getPrincipalAmountAvailableToBorrow calculates $20k available (125k * 80% - $80k = 20k).
Alice borrows $20k.
Alice withdraws her $25k principal.

In the example above, our borrower surpassed the liquidityThresholdPercent, causing the market to be at 100% utilization, essentially locking LPs until a borrow is repaid or another LP deposits.

Impact

Pools will be in a locked state, where every new deposit will be instantly borrowed or used as exit liquidity by another LP. This also break core contract functionality - borrowers should not be able to borrow past liquidityThresholdPercent.

Code Snippet

        require(
            getPrincipalAmountAvailableToBorrow() >= _principalAmount,
            "Invalid loan max principal"
        );

Tool used

Manual Review

Recommendation

This issue relates to the overall structure of the protocol and how it implements its math, and not some specific one-liner issue. I am unable to give an exact solution. I can only mention that deposit/withdraw windows will not work here, as the borrower can just withdraw after the time delay.

ethereumdegen commented 6 months ago

Huh. Well i am not exactly sure what this means or how to fix it. Need some help here.

Are you sure that this prevents lenders from withdrawing? Lenders are supposed to always be able to withdraw funds regardless of the getPrincipalAmountAvailableToBorrower()

Also i think this is fixed by the PR that incorporates the 'amount being borrowed' into this calculation. Right here line 757

https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/23/files

please make sure i am correct..

sherlock-admin2 commented 6 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/23/files

pkqs90 commented 6 months ago

Escalate

I think this issue is duplicate to https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110. The core issue in both issues is users can perform: 1) Deposit, 2) Take a loan, 3) Withdraw in 1 transaction (or in a short period of time).

This issue talks about the above attack path can bypass the borrowing limit, while https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110 talks about the same attack path can be used to toy with interest rate.

The sherlock doc states that if two issues share the same vulnerability, they should be duplicate:

Issues identifying a core vulnerability can be considered duplicates.

Scenario A:
There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack 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.

sherlock-admin3 commented 6 months ago

Escalate

I think this issue is duplicate to https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110. The core issue in both issues is users can perform: 1) Deposit, 2) Take a loan, 3) Withdraw in 1 transaction (or in a short period of time).

This issue talks about the above attack path can bypass the borrowing limit, while https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110 talks about the same attack path can be used to toy with interest rate.

The sherlock doc states that if two issues share the same vulnerability, they should be duplicate:
Issues identifying a core vulnerability can be considered duplicates.

Scenario A:
There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack 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.

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.

nevillehuang commented 6 months ago

Agree with @pkqs90, this issue and duplicates should be a duplicate of #110

0x73696d616f commented 6 months ago

I do not agree with this escalation, the escalator is mixing the attack path with the root cause. It is a coincidence that both issues are described with the same exact attack path, but their root causes are different, which is the important factor here. You can fix one issue that the other issue is still there. The root cause of this issue is not checking the liquidity threshold when withdrawing. Depositing before borrowing and withdrawing is not required. The bug is still here if past borrowers decide to withdraw -> $100k deposits $80k borrows liquidityThresholdPercent is 80%

Some user withdraws 20k $80k deposits $80k borrows liquidityThresholdPercent is 100%

The 2 issues do not even show the same code snippet.

cvetanovv commented 6 months ago

I disagree with the escalation.

The root cause of this issue and #110 are entirely different.

This issue is that a user can bypass the borrowing limit
The #110 root cause is that the user can manipulate the interest rate.

What they have in common is the path of attack. However, in order to decide whether #68 (Medium) should be duplicated with #110 (High), they must have a common root cause.

The Sherlock documentation also records:

The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

This issue fits into all three categories: different impact, implementation, and fix.

Planning to reject the escalation and leave the issue as is.

0x73696d616f commented 6 months ago

I do agree with #44 and #48 being duplicates of #110 though (which is currently the case, so no change).

pkqs90 commented 6 months ago

Hi @0x73696d616f @cvetanovv. First of all, I'd like to make it clear I'm perfectly okay whether this escalation goes through or not, all my comments are based on facts rather than intention. For me, I just want to better understand how the judging process works.

After reading your comments, I still don't understand why this is an non-duplicate issue.

The liquidityThresholdPercent in this protocol is used only for calculating getPrincipalAmountAvailableToBorrow(), which is used for checking whether the amount of principal borrowed exceeds the limit in acceptFundsForAcceptBid().

Yes, users can withdraw their principal and make the liquidity percentage higher than liquidityThresholdPercent, but that is not what this issue is talking about - and I don't think it is a issue anyways, since it doesn't matter if the current liquidity percentage is higher or equal to liquidityThresholdPercent - either way no more principal can be taken as loan.

The root cause this issue is talking about is that borrowers can bypass the liquidityThresholdPercent check by depositing principal before creating a loan, and withdraw the principal after loan is created. This attack path is identical to https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110. Both issues talk about the user can deposit principal before taking action - but with different goals - this issue's goal is to bypass liquidityThresholdPercent check, while https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110 is to lower interest rate.

Again, I'm perfectly fine whether this escalation goes through or not. Maybe I've misunderstood this issue (maybe the author @0x3b33 can give some reference). Just trying to figuring out the duplication logic here. Thanks!

0x73696d616f commented 6 months ago

The root cause this issue is talking about is that borrowers can bypass the liquidityThresholdPercent check by depositing principal before creating a loan, and withdraw the principal after loan is created.

This is not the root cause, and you agree with me given the next sentence

This attack path is identical to https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/110

I do not agree with the next paragraph at all.

Yes, users can withdraw their principal and make the liquidity percentage higher than liquidityThresholdPercent, but that is not what this issue is talking about - and I don't think it is a issue anyways, since it doesn't matter if the current liquidity percentage is higher or equal to liquidityThresholdPercent - either way no more principal can be taken as loan.

The issue is bypassing liquidityThresholdPercent. This issue found an attack path that matches other issues, does not mean they are dups. We would not even be having this conversation if the attack path was just withdrawing to surpass the threshold (much simpler but less complex so would increase the likelihood of being overlooked by a judge imo).

cvetanovv commented 6 months ago

The similarity between the two issues is that they have the same exact attack path. We do not duplicate issues because of the same exact attack path but because of the same root cause.

Soon, this rule will be improved.

I stand by my initial decision to reject the escalation.

Evert0x commented 6 months ago

Result: Medium Has Duplicates

sherlock-admin4 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

pkqs90: rejected

sherlock-admin2 commented 5 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-04-teller-finance-judging

0x3b - Borrowers can surpass `liquidityThresholdPercent` and borrow to near 100% of the principal #68

Borrowers can surpass `liquidityThresholdPercent` and borrow to near 100% of the principal

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-04-teller-finance-judging

0x3b - Borrowers can surpass `liquidityThresholdPercent` and borrow to near 100% of the principal #68

Borrowers can surpass liquidityThresholdPercent and borrow to near 100% of the principal

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Borrowers can surpass `liquidityThresholdPercent` and borrow to near 100% of the principal