Overpowered lender and borrower abilities, can lead to non-liquidatable loans

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L288-L315
https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L911-L917

Vulnerability details

Impact

A single lender can blacklist all other lenders. A single borrower can blacklist all other borrowers. Oppositely a single lender can whitelist any address and a single borrower can whitelist any address.

The most severe impact of such abilities is the fact that only approved lenders can liquidate loans. Therefore, they can remove all other lenders from the approved list and now all debt positions cannot be liquidated.

• If the lender is also an approved borrower, they have a non-liquidatable position.
• Since they are the only lender, they are the only one who can liquidate other borrow positions. This would result in a large profit for the attacker.

Proof of Concept

As stated above, the design allows for any lender or borrower to approve or revoke approvals for any other address. This design relies on the fact that all approved lenders and borrowers will act in good faith. Even a single malicious lender or borrower can blacklist all other lenders or borrower, or oppositely, approve as many addresses as desired.

    function setApprovedLenders(address[] calldata _lenders, bool _approval) external approvedLender(msg.sender) {
        for (uint256 i = 0; i < _lenders.length; i++) {
            // Do not set when _approval == false and _lender == msg.sender
            if (_approval || _lenders[i] != msg.sender) {
                approvedLenders[_lenders[i]] = _approval;
                emit SetApprovedLender(_lenders[i], _approval);
            }
        }
    }

    function setApprovedBorrowers(address[] calldata _borrowers, bool _approval) external approvedBorrower {
        for (uint256 i = 0; i < _borrowers.length; i++) {
            // Do not set when _approval == false and _borrower == msg.sender
            if (_approval || _borrowers[i] != msg.sender) {
                approvedBorrowers[_borrowers[i]] = _approval;
                emit SetApprovedBorrower(_borrowers[i], _approval);
            }
        }
    }

Non-liquidatable loan via approvedLender() modifier after removing all other lenders:

    function liquidate(uint256 _shares, address _borrower)
        external
        whenNotPaused
        nonReentrant
        approvedLender(msg.sender)
        returns (uint256 _collateralForLiquidator)
    {

Tools Used

Manual review.

Recommended Mitigation Steps

I imagine that a centralized party such as the contract admin should have control over such powerful setter functions.

[amirnader-ghazvini]

Duplicate of #157

[gititGoro]

duplicate of #269