Anyone can remove existing term without queueing through `setTerms()`

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/PoolOfferHandler.sol#L110

Vulnerability details

Impact

In PoolOfferHandler, new terms require a two-step process for setting (setTerms() and confirmTerms()). The setTerm() function is onlyOwner, but the confirmTerms() function can be called by anyone. This function uses the provided input __terms from the caller to execute the logic. This could enable an attacker to remove all existing terms, even if the owner does not intend to do so (without pending through the setTerms() function).

function confirmTerms(TermsKey[] calldata _termKeys, Terms[] calldata __terms) external { 
    if (block.timestamp - pendingTermsSetTime < NEW_TERMS_WAITING_TIME) {
        revert TooSoonError();
    }
    for (uint256 i = 0; i < __terms.length; i++) {
        if (_termKeys[i].duration > getMaxDuration) {
            revert InvalidDurationError();
        }
        uint256 pendingAprPremium = _pendingTerms[_termKeys[i].collection][_termKeys[i].duration][_termKeys[i]
            .maxSeniorRepayment][__terms[i].principalAmount];
        // @audit Can be used to remove terms without pending through setTerm()
        if (pendingAprPremium != __terms[i].aprPremium) {
            revert InvalidTermsError();
        }
        _terms[_termKeys[i].collection][_termKeys[i].duration][_termKeys[i].maxSeniorRepayment][__terms[i]
            .principalAmount] = __terms[i].aprPremium;
        delete _pendingTerms[_termKeys[i].collection][_termKeys[i].duration][_termKeys[i]
            .maxSeniorRepayment][__terms[i].principalAmount];
    }
    pendingTermsSetTime = type(uint256).max;

    emit TermsSet(_termKeys, __terms);
}

Proof of Concept

Consider the following scenario

1. The pool has 5 existing terms. Now the owner wants to create a new term in the Pool, so they call setTerms() with the __terms they want to set up. The new term is pending confirmation after a waiting period.
2. After NEW_TERMS_WAITING_TIME, an attacker calls confirmTerms() with _termKeys set to all 5 existing terms and __terms.aprPremium = 0.
3. The function executes with the input provided by the attacker. Since the pendingAprPremium of these terms is reset to 0 after it is confirmed earlier, the check if (pendingAprPremium != __terms[i].aprPremium) is bypassed. The attacker could set the _terms[][][][] mapping of existing loans to 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Only allow the owner to call confirmTerms().

Assessed type

Invalid Validation

[c4-judge]

0xA5DF marked the issue as primary issue

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report

[0xend]

https://github.com/pixeldaogg/florida-contracts/pull/367