Any liquidators can pretend to be a loan contract to validate offers, due to insufficient validation

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

Any liquidators can pretend to be a loan contract to validate offers, due to insufficient validation

Proof of Concept

Accepted callers in a loan manager (e.g. Pool.sol) can be either liquidators or loan contracts.

And only loan contracts should validate offers during a loan creation. However, the current access control check is insufficient in pool::validateOffer, which allows liquidators to pretend to be a loan contract, and directly modify storage (__outstandingValues) bypassing additional checks and accounting in a loan contract.

//src/lib/pools/Pool.sol
    //@audit onlyAcceptedCallers only doesn't ensure caller is a loan contract
|>    function validateOffer(bytes calldata _offer, uint256 _protocolFee) external override onlyAcceptedCallers {
        if (!isActive) {
            revert PoolStatusError();
        }
...

(https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L392)

Current access control check(onlyAcceptedCallers) only ensures caller is accepted caller but doesn't verify the caller is a loan contract (_isLoanContract(caller)==true).

//src/lib/loans/LoanManager.sol
    modifier onlyAcceptedCallers() {
        if (!_acceptedCallers.contains(msg.sender)) {
            revert CallerNotAccepted();
        }
        _;
    }

When a liquidator calls validateOffer, they can provide a fabricated bytes calldata _offer and uint256 _protocolFee bypassing additional checks and state accounting in a loan contract. For example, in MultiSourceLoan.sol- emitLoan, extra checks are implemented on LoanExecutionData to verify borrower and lender signatures and offer expiration timestamp as well as transfer collateral NFT tokens and recoding loan to storage. All of the above can be skipped if a liquidator directly call validateOffer and modify __outstandingValues without token transfer.

Tools Used

Manual

Recommended Mitigation Steps

In Pool::validateOffer, consider adding a check to ensure _isLoanContract(msg.sender)==true

Assessed type

Invalid Validation

[c4-judge]

0xA5DF marked the issue as primary issue

[0xend]

This is a low one given these contracts must all live within our ecosystem and be whitelisted. Given there's alreayd that trust assumption, not doing that check to save some gas on an extra state read.

[C4-Staff]

@0xend Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xA5DF]

This is a low one given these contracts must all live within our ecosystem and be whitelisted

That makes sense. However, I'm judging this based on the info that was present to the wardens in the README and docs. Given that that trust assumption wasn't noted there I'm going to sustain med severity.