Borrower could pay inflated collateral.

Summary

The predict.fun protocol allows for users to have their loan/borrow proposals easily matched with the help of the matchProposals(...) function. However, due to an issue in the way the collateralAmount is updated in the proposal fulfilments, subsequent borrowers could end up paying more collateral than needed.

Root Cause

If we take a closer look into the matchProposals(...) function https://github.com/sherlock-audit/2024-09-predict-fun/blob/main/predict-dot-loan/contracts/PredictDotLoan.sol#L320-L449, we can see that when the proposal fulfilments are updated, there is a discrepancy between what values are given for the actual borrow fulfilment:

https://github.com/sherlock-audit/2024-09-predict-fun/blob/main/predict-dot-loan/contracts/PredictDotLoan.sol#L395-L408

collateralAmountRequired is calculated using the parameters coming from the loanProposal instead of the borrowProposal. Because of this the collateral ratio of the borrower will wrongly increase, leading to a lower collateralAmountRequired in the borrow proposal context.

Lets look at some numbers to further dissect the problem:

Internal pre-conditions

Alice posts borrow proposal (2000 collateral, 1000 loanAmount, 1000 availableAmount)
Bob posts loan proposal (1500 collateral, 1000 loanAmount, 1000 availableAmount)
Naruto acceptBorrowRequest(aliceProposal, 300);
Naruto acceptLoanOffer(bobProposal, 500);
Alice with borrow proposal of (2000 collateral, 1000 loanAmount, 700 availableAmount)
Bob with loan proposal of (1500 collateral, 1000 loanAmount, 500 availableAmount)

External pre-conditions

No response

Attack Path

Let's invoke matchProposals on Alice and Bob.
AvailableAmount will be 500 as 500<700
collateralAmountRequired will be 1500*500/1000 = 750
That means that 750 collateral will be added to the fulfilment of the loan and borrow proposal.
Although that seems correct actually the correct borrow collateral should be calculated this way: 2000*500/1000 = 1000.
That means that we have a discrepancy of around 250 amount.
Now if we have someone to accept Alice's borrow proposal and decide to fill the whole amount of 200 left(700-500 from matchProposals).
She will pay that inflated collateral due to the discrepancy that happened in match proposal.
This is because of this formula in _calculateCollateralAmountRequired()

https://github.com/sherlock-audit/2024-09-predict-fun/blob/main/predict-dot-loan/contracts/PredictDotLoan.sol#L1166-L1167

Impact

Due to the improper collateral calculations subsequent borrowers end up paying inflated collateral

PoC

The following PoC can be added in the PredictDotLoan.matchProposals.t.sol test file. I have added the following test helper function:

function _signProposalBorrower2(IPredictDotLoan.Proposal memory proposal)
        internal
        view
        returns (bytes memory signature)
    {
        signature = _signProposal(proposal, borrower2PrivateKey);
    }

function testInvalidCollateralPaid() public {
        _updateProtocolFeeRecipientAndBasisPoints(200);

        IPredictDotLoan.Proposal memory loanProposal = _generateLoanOffer(IPredictDotLoan.QuestionType.Binary);
        loanProposal.loanAmount = 1000 ether;
        loanProposal.collateralAmount = 1500 ether;
        loanProposal.signature = _signProposal(loanProposal);

        IPredictDotLoan.Proposal memory borrowProposal = 
        _generateBorrowRequest(IPredictDotLoan.QuestionType.Binary);
        borrowProposal.loanAmount = 1000 ether;
        borrowProposal.collateralAmount = 2000 ether;
        borrowProposal.from = borrower;
        borrowProposal.signature = _signProposalBorrower(borrowProposal);

        mockERC20.mint(whiteKnight, 310 ether);
        vm.prank(whiteKnight);
        mockERC20.approve(address(predictDotLoan), 310 ether);

        _mintCTF(whiteKnight);

        vm.startPrank(whiteKnight);
        mockCTF.setApprovalForAll(address(predictDotLoan), true);
        vm.stopPrank();

        vm.startPrank(whiteKnight);
        predictDotLoan.acceptBorrowRequest(borrowProposal, 300 ether);
        predictDotLoan.acceptLoanOffer(loanProposal, 500 ether);
        vm.stopPrank();

        mockERC20.mint(lender, 500 ether);
        vm.prank(lender);
        mockERC20.approve(address(predictDotLoan), 500 ether);

        _mintCTF(borrower);

        predictDotLoan.matchProposals(borrowProposal, loanProposal);

        mockERC20.mint(whiteKnight, 200 ether);
        vm.prank(whiteKnight);
        mockERC20.approve(address(predictDotLoan), 200 ether);

        vm.startPrank(whiteKnight);
        console2.log("CTF balance of borrower before: ", mockCTF.balanceOf(borrower, _getPositionId(true)));
        vm.assertEq(mockCTF.balanceOf(borrower, _getPositionId(true)), 650 ether);
        // whiteKnight accepts the borrow proposal, and the borrower should pay only 400 ether worth of collateral
        predictDotLoan.acceptBorrowRequest(borrowProposal, 200 ether);
        console2.log("CTF balance of borrower after: ", mockCTF.balanceOf(borrower, _getPositionId(true)));
        // borrower should have paid only 400 ether worth of collateral, and should have been left with 250
        // However, due to the invalid calculation he ends up paying 650 ether worth of collateral and loses all of his collateral
        vm.assertEq(mockCTF.balanceOf(borrower, _getPositionId(true)), 0);
    }

Mitigation

Recommended mitigation:

function _calculateCollateralAmountRequired(
        Proposal calldata proposal,
        Fulfillment storage fulfillment,
        uint256 fulfillAmount
    ) private view returns (uint256 collateralAmountRequired) {
-        if (fulfillment.loanAmount + fulfillAmount == proposal.loanAmount) {
-            collateralAmountRequired = proposal.collateralAmount - fulfillment.collateralAmount;
-        } else {
            collateralAmountRequired = (proposal.collateralAmount * fulfillAmount) / proposal.loanAmount;
-        }
    }

sherlock-audit / 2024-09-predict-fun-judging

056Security - Borrower could pay inflated collateral. #245