T1MOH - The submitBid transaction lack of expiration timestamp check

[sherlock-admin]

T1MOH

medium

The submitBid transaction lack of expiration timestamp check

Summary

Submitting bid misses the transaction expiration check, which may lead to receiving principal at a lower price and to collateral being sold at a higher price than the market price at the moment of a submitBid(). Borrowers can receive less than expected for provided collateral.

Vulnerability Detail

The transaction can be pending in mempool for a long time and can be executed in a long time after the user submit the transaction.
Problem is submitBid(), which trusts bid as valid even if market prices of principal and collateral have changed a lot.

        bid.loanDetails.timestamp = uint32(block.timestamp);
        bid.loanDetails.loanDuration = _duration;

Impact

It makes borrower to lose money by submitting disadvantageous bid in worse case. And prevents the borrower from making bids that will be valid for a short period of time in best case.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/TellerV2.sol#L334-L368

Tool used

Manual Review

Recommendation

Use deadline mechanism as in Uniswap V2 contract addLiquidity function implementation
https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L61

function addLiquidity(
	address tokenA,
	address tokenB,
	uint amountADesired,
	uint amountBDesired,
	uint amountAMin,
	uint amountBMin,
	address to,
	uint deadline
) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
	(amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
	address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
	TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
	TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
	liquidity = IUniswapV2Pair(pair).mint(to);
}

modifier ensure(uint deadline) {
	require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
	_;
}

[deadrosesxyz]

Escalate for 10 USDC
Disagree with severity. Issue should be QA/ Low. In worst case scenario, borrowers collateral will be much more valuable than the loan they receive. Upon repaying the loan they will still get their collateral assets back. There would be no losses.
Furthermore, cancelBid exists for a reason. Forgetting to call it when a user doesn't want to execute a bid anymore would be the same as forgetting to pay a loan repayment and getting defaulted.

[sherlock-admin]

Escalate for 10 USDC
Disagree with severity. Issue should be QA/ Low. In worst case scenario, borrowers collateral will be much more valuable than the loan they receive. Upon repaying the loan they will still get their collateral assets back. There would be no losses.
Furthermore, cancelBid exists for a reason. Forgetting to call it when a user doesn't want to execute a bid anymore would be the same as forgetting to pay a loan repayment and getting defaulted.

You've created a valid escalation for 10 USDC!

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.

[T1MOH593]

Escalate for 10 USDC
This is valid.
Disagree with escalation specifically with statement "Upon repaying the loan they will still get their collateral assets back. There would be no losses." Losses will be, suppose a different scenario.

1. User submits bid to borrow 10 BTC on 1 year at 10% APR. On time of sending transaction BTC costs $10k. Supposed loan interest to repay will be 10BTC * $10k * 10% = $10k

2. But specified gas price is too low to be mined

3. When submitBid transaction is mined BTC price went higher to $20k.

4. MEV detects this tx and backruns with lenderAcceptBid()

5. Actual loan interest to repay is 10BTC * $20k * 10% = $20k. But user supposed to repay $10k
   This violates the statement "There would be no losses for user".

[sherlock-admin]

Escalate for 10 USDC
This is valid.
Disagree with escalation specifically with statement "Upon repaying the loan they will still get their collateral assets back. There would be no losses." Losses will be, suppose a different scenario.

1. User submits bid to borrow 10 BTC on 1 year at 10% APR. On time of sending transaction BTC costs $10k. Supposed loan interest to repay will be 10BTC * $10k * 10% = $10k

2. But specified gas price is too low to be mined

3. When submitBid transaction is mined BTC price went higher to $20k.

4. MEV detects this tx and backruns with lenderAcceptBid()

5. Actual loan interest to repay is 10BTC * $20k * 10% = $20k. But user supposed to repay $10k
   This violates the statement "There would be no losses for user".

You've created a valid escalation for 10 USDC!

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.

[deadrosesxyz]

When creating a loan, the collateral expected to be would be of relatively same value. In the described case where the principal asset has jumped 2x in value, it would become worth more than the given collateral. If the collateral costs less than the principal, the user has no incentive to repay the loan. Therefore, if a case like the described above happens, user could even profit from it as they can just not repay the loan at all. If the principal asset has significantly increased in value, there is no incentive for the lender to accept the loan.

[Trumpero]

Agree that it is a low/info issue

[ethereumdegen]

Github PR: Issue 187 - Add deadline to submit bid

[ethereumdegen]

When re-reviewing this issue, I noticed that we actually already have an expiration timestamp check for bids.

    require(!isLoanExpired(_bidId), "Bid has expired");

So no fix will be done here.

[hrishibhat]

Escalation accepted

Valid low
There is no valid attack path or loss of funds shown here.

[sherlock-admin]

Escalation accepted

Valid low
There is no valid attack path or loss of funds shown here.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.