Approved lenders and borrowers can allow and block any arbitrary address

[code423n4]

Lines of code

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

Vulnerability details

Impact

Whitelisted addresses are able to deploy customized pairs. One of the customizations is the enforcement of a lender and borrower allowlist and blocklist by providing an array of approved borrowers _approvedBorrowers and/or an array of the approved lenders _approvedLenders.

Lender and borrowers can be approved/blocked by invoking the functions FraxlendPair.setApprovedLenders or FraxlendPair.setApprovedBorrowers. Only approved lenders, as well as approved borrowers, are able to call those functions.

However, an approved lender is able to approve and block any other arbitrary address. Same for borrowers. If any of the approved lenders or borrowers turn dishonest and malicious, this single actor is able to front-run and block all other addresses and prevent them from using the deployed pair.

Blocked lenders are not able to use the following functions:

• FraxlendPairCore.deposit
• FraxlendPairCore.mint
• FraxlendPairCore.liquidate
• FraxlendPairCore.liquidateClean

Blocked borrowers are not able to use the following functions:

• FraxlendPairCore.borrowAsset
• FraxlendPairCore.leveragedPosition

Proof of Concept

FraxlendPair.sol#L288-L296

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);
        }
    }
}

FraxlendPair.sol#L307-L315

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);
        }
    }
}

Tools Used

Manual review

Recommended mitigation steps

Consider allowing only the owner of the deployed pair to allow/block addresses. Otherwise, the list of approved lenders/borrowers can grow very fast and the chance of having a bad actor as one of the approved addresses, is very high. It only takes a single bad actor to denial-of-service the deployed pair.

[0xA5DF]

Duplicate of #135

[DrakeEvans]

This is the intended design, approvedBorrower and approvedLenders are meant to represent a single entity. Like a single DAO, the ability to have multiple addresses is a convenience but ultimately the responsibility of each side of the contract lies with the borrower or lender.

[gititGoro]

Both the sponsor and the wardens have solid arguments to make in support of this issue being either valid or invalid.
On the one hand, this feature is by design as the sponsor has pointed out and is not in practice open to abuse. For this reason, it could be considered invalid. However, the wardens are not wrong to point out that giving peer contracts co-admin rights presents a horizontal risk.

Ultimately the risk is largely mitigated by the fact that the entrance into becoming an approved entity is via the initial whitelist. As long as the deployer has tight control over the initial whitelist, this should be the case.

Nonetheless, the horizontal risk is amplified with every newly approved lender/borrower. It only takes one mistake to destroy the whole system. So it would be best practice to employ a hierarchy of admin granting such as owner->approved whitelisting.

This issue and all the duplicates will be converted to QA and linked to existing QA issues submitted by these wardens.

TL;DR It is not good practice to allow for horizontal admin granting but it suits the need of the Frax DAO and the sponsor has intentionally chosen this design. The feature will be treated as a security smell