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

Upgraded Q -> 3 from #35 [1713794205670] #87

Closed
c4-judge opened this issue Apr 22, 2024 · 3 comments
Closed

Upgraded Q -> 3 from #35 [1713794205670] #87

c4-judge opened this issue Apr 22, 2024 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly nullified Issue is high quality, but not accepted

Comments

@c4-judge
Copy link
Contributor

Judge has assessed an item in Issue #35 as 3 risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

The attackers make it impossible for borrowers to repay their debts, and the collateral is liquidated when the debts mature.

Proof of Concept

repayLoan, need to check the loanId, if the id is inconsistent will revert.

    function repayLoan(LoanRepaymentData calldata _repaymentData) external override nonReentrant {
        uint256 loanId = _repaymentData.data.loanId;
        Loan calldata loan = _repaymentData.loan;
        .....
@>      _baseLoanChecks(loanId, loan);
        .....
    }
    
    function _baseLoanChecks(uint256 _loanId, Loan memory _loan) private view {
        if (_loan.hash() != _loans[_loanId]) {
            revert InvalidLoanError(_loanId);
        }
        if (_loan.startTime + _loan.duration < block.timestamp) {
            revert LoanExpiredError();
        }
    }

The problem is that _loans[_loanId] can change, for example, when mergeTranches delete the old loanId and write the new one.

    _loans[loanId] = loanMergedTranches.hash();
    delete _loans[_loanId];

An attacker can use the front-running attack method, when repayLoan is called, execute the mergeTranches function in advance, and make the id in _loans updated. In this case, the repayLoan execution will fail due to inconsistent _loanId.

If the attacker keeps using this attack, the borrower's debt will not be repaid, eventually causing the collateral to be liquidated.

In addition to the mergeTranches function, the attacker can also call addNewTranche, and the borrower can also call the refinance-related function, again causing _loanId to be updated.

An attacker can also use the same method to attack refinance related functions, making refinance unable to execute.

An attacker can also use the same method to attack the liquidateLoan function, making it impossible for debts to be cleared.

Tools Used

vscode, manual

Recommended Mitigation Steps

Do not delete _loanId

Assessed type

DoS

@c4-judge c4-judge added the 3 (High Risk) Assets can be stolen/lost/compromised directly label Apr 22, 2024
@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Apr 22, 2024
@c4-judge
Copy link
Contributor Author

0xA5DF marked the issue as selected for report

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Apr 22, 2024
@c4-judge
Copy link
Contributor Author

0xA5DF marked the issue as satisfactory

@c4-judge c4-judge added nullified Issue is high quality, but not accepted and removed satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report labels Apr 22, 2024
@c4-judge
Copy link
Contributor Author

0xA5DF marked the issue as nullified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly nullified Issue is high quality, but not accepted
Projects
None yet
Development

No branches or pull requests

1 participant