c4-bot-1 commented 3 months ago

Lines of code

Vulnerability details

C4 Issue

M-10: Any liquidators can pretend to be a loan contract to validate offers, due to insufficient validation

Comments

Original vulnerabilities/impacts: The issue is pool::validateOffer has an onlyAcceptedCaller modifier but this modifier doesn’t distinguish between a liquidator contract or a loan contract and will allow either a liquidator contract or a loan contract to pass.

It can be argued that it’s risky to allow a liquidator to call loanContract-only functions, because a liquidator doesn’t have to implement any checks in a loan contract and can fabricate a loan offer to be written in storage.

Mitigation

Fix: https://github.com/pixeldaogg/florida-contracts/pull/397/files

//src/lib/pools/Pool.sol
    function validateOffer(bytes calldata _offer, uint256 _protocolFee) external override {
        if (!_isLoanContract[msg.sender]) {
            revert CallerNotAccepted();
        }
...

The mitigation is that in Pool::validateOffer, caller/msg.sender is explicitly checked to be a loan contract, which eliminates the attack vector and resolve the issue.

However, it should be noted that Pool::loanRepayment is also at risk but currently not mitigated. Pool::loanRepayment is only intended by called by a loan Contract, but it currently lacks the check that the caller is a loan contract, not a liquidator. Similarly, a liquidator can still call loanRepayment to modify queue and outstandingValues accounting directly (_loanTermination()). Due to the sponsor’s comment on the original issue that all accepted callers are trusted, I consider not updating the access control in loanRepayment to be low risks / informational.

Conclusion

LGTM

c4-judge commented 3 months ago

alex-ppg marked the issue as satisfactory