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

10 stars 9 forks source link

psb01 - Borrower could be paying more amount than owed. #228

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

psb01

medium

Borrower could be paying more amount than owed.

Summary

There is situation when loan amount paid by borrower could be more than owed amount to the lender.

Vulnerability Detail

Inside TellerV2.sol, In function _repayLoan at line L866-867 there is check that if paymentAmount >= _owedAmount then paymentAmount = _owedAmount but later on the payment which is transferred to lender or send to escrowVault by function _sendOrEscrowFunds is the same Payment struct with payments amount (principal + interest) irrespective of whether paymentAmount > owedAmount or not.

Impact

This can cause borrower to pay more than amount they owe to the lender.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L851-L898

Tool used

Manual Review

Recommendation

A simple fix could be if (paymentAmount >= _owedAmount) _payment.principal = _owedAmount - _payment.interest

nevillehuang commented 3 months ago

Invalid, duplicate of #32, insufficient impact described, and if a user overpay, that would be his responsibility.

psb-01 commented 3 months ago

Escalate Hi, Thanks for comment. I think this should be a valid duplicate of issue #32. I understand that as shown in #32 I didn't describe the DOS part of impact but still I have identified the root cause of the issue as mentioned in my report- but later on the payment which is transferred to lender or send to escrowVault by function _sendOrEscrowFunds is the same Payment struct with payments amount (principal + interest) irrespective of whether paymentAmount > owedAmount or not. which is similar to the selected report- However, the _payment memory struct remains the same. Meaning that if the user has inputted more than supposed to, it will go through also As commented by @nevillehuang if a user overpay, that would be his responsibility. but I would like to mention that in #32 also the whole issue is because the user attempts to overpay

sherlock-admin3 commented 3 months ago

Escalate Hi, Thanks for comment. I think this should be a valid duplicate of issue #32. I understand that as shown in #32 I didn't describe the DOS part of impact but still I have identified the root cause of the issue as mentioned in my report- but later on the payment which is transferred to lender or send to escrowVault by function _sendOrEscrowFunds is the same Payment struct with payments amount (principal + interest) irrespective of whether paymentAmount > owedAmount or not. which is similar to the selected report- However, the _payment memory struct remains the same. Meaning that if the user has inputted more than supposed to, it will go through also As commented by @nevillehuang if a user overpay, that would be his responsibility. but I would like to mention that in #32 also the whole issue is because the user attempts to overpay

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 3 months ago

This issue should remain invalid per sherlock rules:

In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

There is no impact here described that matches sherlocks criteria for medium severity

psb-01 commented 3 months ago

ok...seems fair to me.

cvetanovv commented 3 months ago

I agree with the Lead Judge comment. There is a lack of quality description of the impact.

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

Evert0x commented 3 months ago

Result: Invalid Unique

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: