bughuntoor - Borrower can send the repayment with less gas and it will repayLoanCallback will not be executed.

[sherlock-admin3]

bughuntoor

high

Borrower can send the repayment with less gas and it will repayLoanCallback will not be executed.

Summary

Borrower can send the repayment with less gas and it will repayLoanCallback will not be executed

Vulnerability Detail

When repaying a loan, if there's loan repayment listener set, it attempts to call the repayLoanCallback in a try-catch, passing 80,000 gas for the call

        if (loanRepaymentListener != address(0)) {
            try
                ILoanRepaymentListener(loanRepaymentListener).repayLoanCallback{
                    gas: 80000
                }( //limit gas costs to prevent lender griefing repayments
                    _bidId,
                    _msgSenderForMarket(bid.marketplaceId),
                    _payment.principal,
                    _payment.interest
                )
            {} catch {}

If at the time of entering the try-catch statement, the gas left is less than 80,000, it will simply assign 63/64th of it for the repayLoanCallback. If that gas is not enough, it will simply enter the catch statement.

This allows for users both purposefully and accidentally sending the tx with just enough gas so that it is not enough to execute the repayLoanCallback and it enters the catch statement.

Since LenderCommitmentGroup_Smart.sol depends on the execution of the callback for the sake of proper accounting, the described attack path will break all accounting and result as stuck funds (as the contract will assume the funds are still not repaid)

Impact

Broken accounting, permanently stuck funds

Code Snippet

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

Tool used

Manual Review

Recommendation

before calling repayLoanCallback, make sure there's at least 80,000 gasleft()

[ethereumdegen]

"If at the time of entering the try-catch statement, the gas left is less than 80,000, it will simply assign 63/64th of it for the repayLoanCallback. If that gas is not enough, it will simply enter the catch statement."

Where does this 63/64 come from ? Is there some property about the try/catch statement in solidity i am not aware of ? i would like to read more about it

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
teller-protocol/teller-protocol-v2-audit-2024#12

[spacegliderrrr]

@ethereumdegen Hey, quoting EIP-150 directly here:

If a call asks for more gas than the maximum allowed amount (i.e. the total amount of gas remaining in the parent after subtracting the gas cost of the call and memory expansion), do not return an OOG error; instead, if a call asks for more gas than all but one 64th of the maximum allowed amount, call with all but one 64th of the maximum allowed amount of gas (this is equivalent to a version of EIP-90ethereum/EIPs#90 plus EIP-114ethereum/EIPs#114). CREATE only provides all but one 64th of the parent gas to the child call.

link

[0xadrii]

This attack is not possible.

Setting the gass so that it is lower than 80000 when entering the try-catch will not allow the rest of the transaction to execute, hence, the whole transaction will revert. As you say, 63/64 will be forwarded, leaving only 1/64 for the rest of the whole transaction to execute. Even if you sent the tx so that a strict 80000 gas remains, only 1250 units of gas would remain for the whole transaction to execute, which is not enough, hence reverting and making the attack impossible.

[ethereumdegen]

I think the fix for issue for #178 deals with this anyways .

[nevillehuang]

@spacegliderrrr @0xadrii Is a PoC possible for this issue to verify this comment?. I am assuming you guys mean the following lines of code here would revert anyways given the small amount of 1250 gas remaining, which seems likely.

[0xadrii]

Hey @nevillehuang , exactly.

As seen here, changing a storage value has a fixed cost of 2900 units of gas if we're writing to a "clean slot" (i.e, a slot that has not yet been updated in the current execution context). This is our case, given that bid.loanDetails.totalRepaid.principal, bid.loanDetails.totalRepaid.interest and bid.loanDetails.lastRepaidTimestamp are never changed in the current execution flow until these lines, so these are clean slots.

This means that only changing one variable in storage would already surpass by far the 1250 units of gas remaining for the attack to be feasible.
In our case, we're actually updating three storage slots (corresponding to bid.loanDetails.totalRepaid.principal, bid.loanDetails.totalRepaid.interest and bid.loanDetails.lastRepaidTimestamp), so the required gas to finish the transaction gracefully should be at least of 8700 (2900 * 3).

Moreover, the LenderCommitmentGroup_Smart's repayLoanCallback function actually only consumes around 40000 units of gas (even if 80000 are hardcoded and forwarded to it by Teller). This means that in order to perform the attack and force repayLoanCallback to fail, we actually need to build the transaction sending a gas amount so that the call to repayLoanCallback forwards less than 40000 units of gas, which would leave an even smaller gas amount for the rest of the transaction to succeed.

[nevillehuang]

@0xadrii Thanks for the clarification, closing the issue for now, given I believe it requires a PoC given complexities involved.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.