Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOS all Pool's offer through capacity=0 #25

Closed
c4-bot-10 opened this issue Apr 16, 2024 · 7 comments
Closed

DOS all Pool's offer through capacity=0 #25

c4-bot-10 opened this issue Apr 16, 2024 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/loans/MultiSourceLoan.sol#L124

Vulnerability details

Vulnerability details

If offer.capacity=0, then this offer.offerId becomes one-time.

emitLoan()->_processOffersFromExecutionData()

    function _processOffersFromExecutionData(
        address _borrower,
        address _principalReceiver,
        address _principalAddress,
        address _nftCollateralAddress,
        uint256 _tokenId,
        uint256 _duration,
        OfferExecution[] calldata _offerExecution
    ) private returns (uint256, uint256[] memory, Loan memory, uint256) {
...

            _handleProtocolFeeForFee(
                offer.principalAddress, lender, fee.mulDivUp(protocolFee.fraction, _PRECISION), protocolFee.recipient
            );

            ERC20(offer.principalAddress).safeTransferFrom(lender, _principalReceiver, amount - fee);
            if (offer.capacity > 0) {
                _used[lender][offer.offerId] += amount;
            } else {
@>              isOfferCancelled[lender][offer.offerId] = true;
            }

            offerIds[i] = offer.offerId;
            unchecked {
                ++i;
            }
        }
        Loan memory loan = Loan(
            _borrower,
            _tokenId,
            _nftCollateralAddress,
            _principalAddress,
            totalAmount,
            block.timestamp,
            _duration,
            tranche,
            protocolFee.fraction
        );

        return (loanId, offerIds, loan, totalFee);
    }

This gives a malicious attacker an opportunity to maliciously attack all offers with lender == Pool.
Example
Bob call emitLoan(lender == Pool, offerId = 123)

  1. Alice front-run call emitLoan(lender == Pool, offerId = 123,capacity=0, duration=0)
    • after execute , isOfferCancelled[Pool][123] = true
  2. Bob's tranaction will fail, because isOfferCancelled[Pool][123] = true;
  3. Alice call repayLoan() get back nft

Impact

DOS all Pool's offer

Recommended Mitigation

If lender is LoanManager, then offer.capacity must not be 0.

    function _validateOfferExecution(
        OfferExecution calldata _offerExecution,
        uint256 _tokenId,
        address _lender,
        address _offerer,
        bytes calldata _lenderOfferSignature,
        uint256 _feeFraction,
        uint256 _totalAmount
    ) private {
...

+      if (getLoanManagerRegistry.isLoanManager(_lender) && offer.capacity==0) {
+          revert InvalidTrancheError();
+      }

       if ((offer.capacity > 0) && (_used[_offerer][offer.offerId] + _offerExecution.amount > offer.capacity)) { 
            revert MaxCapacityExceededError();
        }

        _checkValidators(_offerExecution.offer, _tokenId);
    }

Assessed type

DoS

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 16, 2024
c4-bot-2 added a commit that referenced this issue Apr 16, 2024
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 18, 2024
@c4-judge
Copy link
Contributor

0xA5DF marked the issue as primary issue

@0xA5DF
Copy link

0xA5DF commented Apr 18, 2024

Sponsor: Please comment on the severity of the issue
If issuing a new offer is a no brainer then this falls under this category - code-423n4/org#143

@0xend
Copy link

0xend commented Apr 19, 2024

It does fall within that category. Attacker would need to be emitting loans + repaying loans to constantly DoS.

@0xend 0xend added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Apr 19, 2024
@c4-judge
Copy link
Contributor

0xA5DF changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Apr 20, 2024
@0xA5DF
Copy link

0xA5DF commented Apr 20, 2024

Marking as low, open to hear arguments for med

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Apr 21, 2024
@c4-judge
Copy link
Contributor

0xA5DF marked the issue as grade-c

@0xA5DF
Copy link

0xA5DF commented Apr 21, 2024

Moved to #74

@0xA5DF 0xA5DF mentioned this issue Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants