c4-bot-9 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/LiquidationDistributor.sol#L32

Vulnerability details

Impact

An attacker can use the distribute function to call Pool#loanLiquidation to add any number of interest/fee to the Pool.

Proof of Concept

The loanLiquidation function in Pool requires permission to access:

    function loanLiquidation(
       ....
@>  ) external override onlyAcceptedCallers {
        uint256 netApr = _netApr(_apr, _protocolFee);
        uint256 interestEarned = _principalAmount.getInterest(netApr, block.timestamp - _startTime);
        uint256 fees = IFeeManager(getFeeManager).processFees(_received, 0);
        getCollectedFees += fees;
        _loanTermination(msg.sender, _loanId, _principalAmount, netApr, interestEarned, _received - fees);
    }

    modifier onlyAcceptedCallers() {
        if (!_acceptedCallers.contains(msg.sender)) {
            revert CallerNotAccepted();
        }
        _;
    }

modifier onlyAcceptedCallers indicates thatmsg.sender needs to be added to the trust list before it can be called.

The distribute function in the LiquidationDistributor contract is a public function and calls Pool#loanLiquidation:

distribute->_handleTrancheExcess->_handleLoanManagerCall:

  function distribute(uint256 _proceeds, IMultiSourceLoan.Loan calldata _loan) external {
        ......
        if (_proceeds > totalPrincipalAndPaidInterestOwed + totalPendingInterestOwed) {
            for (uint256 i = 0; i < _loan.tranche.length;) {
                IMultiSourceLoan.Tranche calldata thisTranche = _loan.tranche[i];
@>                _handleTrancheExcess(
                    _loan.principalAddress,
                    thisTranche,
                    msg.sender,
                    _proceeds,
                    totalPrincipalAndPaidInterestOwed + totalPendingInterestOwed
                );
                unchecked {
                    ++i;
                }
            }
        } else {
            for (uint256 i = 0; i < _loan.tranche.length && _proceeds > 0;) {
                IMultiSourceLoan.Tranche calldata thisTranche = _loan.tranche[i];
@>              _proceeds = _handleTrancheInsufficient(
                    _loan.principalAddress, thisTranche, msg.sender, _proceeds, owedPerTranche[i]
                );
                unchecked {
                    ++i;
                }
            }
        }
    }

    function _handleTrancheExcess(....) private {
        uint256 excess = _proceeds - _totalOwed;
        /// Total = principal + accruedInterest +  pendingInterest + pro-rata remainder
        uint256 owed = _tranche.principalAmount + _tranche.accruedInterest
            + _tranche.principalAmount.getInterest(_tranche.aprBps, block.timestamp - _tranche.startTime);
        uint256 total = owed + excess.mulDivDown(owed, _totalOwed);
@>      _handleLoanManagerCall(_tranche, total);
        ERC20(_tokenAddress).safeTransferFrom(_liquidator, _tranche.lender, total);
    }

    function _handleLoanManagerCall(IMultiSourceLoan.Tranche calldata _tranche, uint256 _sent) private {
        if (getLoanManagerRegistry.isLoanManager(_tranche.lender)) {
@>          LoanManager(_tranche.lender).loanLiquidation(
                _tranche.loanId,
                _tranche.principalAmount,
                _tranche.aprBps,
                _tranche.accruedInterest,
                0,
                _sent,
                _tranche.startTime
            );
        }
    }

The distribute function has no permission control, and an attacker can construct appropriate parameters to call Pool#loanLiquidation and pass in arbitrary parameters. In this way, the interest/fee in the Pool can be arbitrarily set.

Tools Used

vscode, manual

Recommended Mitigation Steps

AuctionLoanLiquidator#settleAuction calls the distribute function:

    function settleAuction(Auction calldata _auction, IMultiSourceLoan.Loan calldata _loan) external nonReentrant {
        .....
        _liquidationDistributor.distribute(proceeds, _loan);
        .....
    }

We should restrict the distribute function to only being called by Liquidator:

-  function distribute(uint256 _proceeds, IMultiSourceLoan.Loan calldata _loan) external {
+  function distribute(uint256 _proceeds, IMultiSourceLoan.Loan calldata _loan) external onlyLiquidator{

Assessed type

Access Control

c4-judge commented 7 months ago

0xA5DF marked the issue as duplicate of #64