Skip to content

Latest commit

 

History

History
103 lines (73 loc) · 4.99 KB

031.md

File metadata and controls

103 lines (73 loc) · 4.99 KB

Custom Pineapple Newt

Medium

Overpaying a loan can DoS an entire lender commitment group

Summary

Summary

Inserting larger amount in repayLoan allows to repay more than what was lent to user, disturbing the internal accounting of a lender group

Description

Lender commitment groups use 2 variables to track how many tokens are currently being lent out - totalPrincipalTokensLended and totalPrincipalTokensRepaid. Effectively, the latter should always be smaller or equal than the former. Whenever the 2 variables are equal it means that there are currently no active loans which is used in getTotalPrincipalTokensOutstandingInActiveLoans

    function getTotalPrincipalTokensOutstandingInActiveLoans()
        public
        view
        returns (uint256)
    {
        return totalPrincipalTokensLended - totalPrincipalTokensRepaid;
    }

This method can be forced to always revert if user repays at least 1 wei more than they were originally lent, causing an underflow during substraction. The code currently allows for repaying more due to a logic error within _repayLoan The method is passed a Payment struct which consists of 2 variables whose sum totals the amount borrower is willing to overpay (principal + interest)

    function _repayLoan(
        uint256 _bidId,
    @>  Payment memory _payment,
        uint256 _owedAmount,
        bool _shouldWithdrawCollateral
    ) internal virtual {
        Bid storage bid = bids[_bidId];
@>      uint256 paymentAmount = _payment.principal + _payment.interest; // memory variable summing the 2 values

        RepMark mark = reputationManager.updateAccountReputation(
            bid.borrower,
            _bidId
        );
        // Check if we are sending a payment or amount remaining
        if (paymentAmount >= _owedAmount) {
@>           paymentAmount = _owedAmount;                               // memory variable is overwritten to match the owed amount

            if (bid.state != BidState.LIQUIDATED) {
                bid.state = BidState.PAID;
            }
         
            _borrowerBidsActive[bid.borrower].remove(_bidId);

       
            if (_shouldWithdrawCollateral) { 
               
                collateralManager.withdraw(_bidId);
            }

            emit LoanRepaid(_bidId);
        } else {
            emit LoanRepayment(_bidId);
        }
@>       _sendOrEscrowFunds(_bidId, _payment);  // @audit-issue unmodified payment struct is passed, memory variable paymentAmount not used anywhere
    }

The paymentAmount >= _owedAmount check is not performed correctly since paymentAmount is practically not used anywhere further down in the code. Unmodified _payment struct is passed in _sendOrEscrowFunds which triggers the repayLoanCallback for the lender group incrementing totalPrincipalTokensRepaid by the amount in the _payment struct.

Once totalPrincipalTokensRepaid is higher than totalPrincipalTokensLended, any attempts to issue loans will revert due to getTotalPrincipalTokensOutstandingInActiveLoans underflowing. Attack can be performed by backrunning commitment group creations and funding by taking a loan and repaying it immediately with an extra wei, bricking it forever since the underflowing function is used in getMinInterestRate which is called in acceptFundsForAcceptBid

Similar finding was part of the previous audit contest yet it was not fixed. Risk of DoS is still very much present and fix is rather simple. Furthermore the README states an invariant that should hold:

Very importantly, we expect the following invariant: the 'EstimatedTotalValue' of a LenderGroupPool should accurately track the value of principal tokens in a pool.

Even if a DoS is not performed, the value of principal tokens will not be tracked correctly since overpays will underestimate the free funds within the contract, lowering the utilization ratio too.

Root Cause

  • _repayLoan does not indicate whether a payment is the last one for a loan
  • sendOrEscrowFunds performs a sanity check incorrectly and uses unmodified payment struct

Internal pre-conditions

None

External pre-conditions

None

Attack Path

  1. Lender commitment group is deployed and funded by owner
  2. Adversary takes a loan for 100e6 USDC, immediately repays it through `repayLoan(bidId, 100e6+1)
  3. Lender commitment group can no longer issue loans since acceptFundsForAcceptBid relies on getTotalPrincipalTokensOutstandingInActiveLoans which now underflows due to 100e6 - (100e6+1) < 0

Impact

DoS of entire group

PoC

No response

Mitigation

If user invokes repayLoan with amount exceeding his total dues, perform repayLoanFull instead.