c4-bot-6 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Compensate.sol#L42

Vulnerability details

Impact

A user can transfer an OVERDUE loan to another user, while he shouldn't be able to. In the Size docs it clearly states:

3. Side note for overdue credit

- The easiest solution is to add an additional criteria to the list above, preventing the overdue credit transfer

Additionally, there are checks throughout the whole codebase that prevent that. More specifically, in the compensate function there is a check that only allows the transfers of ACTIVE loans, meaning no OVERDUE or REPAID loans:

Compensate.sol

    function validateCompensate(State storage state, CompensateParams calldata params) external view {
        CreditPosition storage creditPositionWithDebtToRepay =
            state.getCreditPosition(params.creditPositionWithDebtToRepayId);
        DebtPosition storage debtPositionToRepay =
            state.getDebtPositionByCreditPositionId(params.creditPositionWithDebtToRepayId);
            .
            .
@>          if (!state.isCreditPositionTransferrable(params.creditPositionToCompensateId)) {
                revert Errors.CREDIT_POSITION_NOT_TRANSFERRABLE(
                    params.creditPositionToCompensateId,
                    state.getLoanStatus(params.creditPositionToCompensateId),
                    state.collateralRatio(debtPositionToCompensate.borrower)
                );
            }
            .
            .

    function isCreditPositionTransferrable(State storage state, uint256 creditPositionId)
        internal
        view
        returns (bool)
    {
@>        return state.getLoanStatus(creditPositionId) == LoanStatus.ACTIVE
            && !isUserUnderwater(state, state.getDebtPositionByCreditPositionId(creditPositionId).borrower);
    }

    function getLoanStatus(State storage state, uint256 positionId) public view returns (LoanStatus) {
        // First, assumes `positionId` is a debt position id
        DebtPosition memory debtPosition = state.data.debtPositions[positionId];
        if (isCreditPositionId(state, positionId)) {
            // if `positionId` is in reality a credit position id, updates the memory variable
            debtPosition = getDebtPositionByCreditPositionId(state, positionId);
        } else if (!isDebtPositionId(state, positionId)) {
            // if `positionId` is neither a debt position id nor a credit position id, reverts
            revert Errors.INVALID_POSITION_ID(positionId);
        }

        if (debtPosition.futureValue == 0) {
            return LoanStatus.REPAID;
        } else if (block.timestamp > debtPosition.dueDate) {
@>          return LoanStatus.OVERDUE;
        } else {
            return LoanStatus.ACTIVE;
        }
    }

A user is able though to bypass that check by calling the transaction 1 block before the loan expires, essentially making the receiver to receive an OVERDUE loan.

Proof of Concept

Consider the following scenario:

James is a lender in loanA and a borrower in loanB.
James realizes that loanA is about to become OVERDUE.
James uses the compensate function in the last block and pays his loanB debt with loanA's credit and essentially cashes out.
loanB's lender received an OVERDUE loan when it shouldn't be possible.

This scenario is played out in the following test:

    function test_transfer_overdue_loan() public {
        _deposit(bob, usdc, 100e6);
        _deposit(bob, weth, 100e18);
        _deposit(alice, weth, 100e18);
        _deposit(alice, usdc, 100e6);
        _deposit(james, usdc, 100e6);
        _deposit(james, weth, 100e18);

        uint256[] memory tenors = new uint256[](1);
        tenors[0] = 365 days;

        int256[] memory aprs = new int256[](1);
        aprs[0] = 0.1e18;

        uint256[] memory marketRateMultipliers = new uint256[](1);
        assertTrue(_state().alice.user.borrowOffer.isNull());
        _sellCreditLimit(bob, YieldCurve({tenors: tenors, aprs: aprs, marketRateMultipliers: marketRateMultipliers}));
        _buyCreditLimit(
            alice,
            block.timestamp + 365 days,
            YieldCurve({tenors: tenors, aprs: aprs, marketRateMultipliers: marketRateMultipliers})
        );

        // James becomes lender
        uint256 debtPositionId = _buyCreditMarket(james, bob, RESERVED_ID, 100e6, 365 days, true);
        uint256 creditPositionId = size.getCreditPositionIdsByDebtPositionId(debtPositionId)[0];
        uint256 credit = size.getCreditPosition(creditPositionId).credit;

        // James becomes borrower for the same credit amount as he lended
        uint256 debtPositionId2 = _sellCreditMarket(james, alice, RESERVED_ID, credit, 365 days, true);
        uint256 creditPositionId2 = size.getCreditPositionIdsByDebtPositionId(debtPositionId2)[0];
        uint256 credit2 = size.getCreditPosition(creditPositionId2).credit;

        // Confirm both loans have the same value
        assertEq(credit, credit2);

        // Forward time to 1 second before loans become overdue
        vm.warp(block.timestamp + 365 days - 1);

        assertEq(size.getCreditPosition(creditPositionId).lender, james);

        // James is able to compensate with no reverts and errors
        _compensate(james, creditPositionId2, creditPositionId);

        // Confirm that Alice is now the lender of the OVERDUE loan
        assertEq(size.getCreditPosition(creditPositionId).lender, alice);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a check that the remaining duration of the loan is above a value of your choice, could be the minTenor value:

    function validateCompensate(State storage state, CompensateParams calldata params) external view {
        CreditPosition storage creditPositionWithDebtToRepay =
            state.getCreditPosition(params.creditPositionWithDebtToRepayId);
        DebtPosition storage debtPositionToRepay =
            state.getDebtPositionByCreditPositionId(params.creditPositionWithDebtToRepayId);
        .
        .
        } else {
            CreditPosition storage creditPositionToCompensate =
                state.getCreditPosition(params.creditPositionToCompensateId);
            DebtPosition storage debtPositionToCompensate =
                state.getDebtPositionByCreditPositionId(params.creditPositionToCompensateId);
+           if (
+               state.getDebtPositionByCreditPositionId(params.creditPositionToCompensateId).dueDate - block.timestamp
+                   < state.riskConfig.minTenor
+           ) {
+               revert("REMAINING_DURATION_TOO_SMALL");
+           }
            if (!state.isCreditPositionTransferrable(params.creditPositionToCompensateId)) {
                revert Errors.CREDIT_POSITION_NOT_TRANSFERRABLE(
                    params.creditPositionToCompensateId,
                    state.getLoanStatus(params.creditPositionToCompensateId),
                    state.collateralRatio(debtPositionToCompensate.borrower)
                );
            }
            .
            .
    }

Assessed type

Invalid Validation

hansfriese commented 2 months ago

Duplicate of #56.

c4-judge commented 2 months ago

hansfriese marked the issue as duplicate of #56