Fraxlend (Frax Finance) contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

The ownership of a deployed Fraxlend pair is transferred to COMPTROLLER_ADDRESS on deployment via FraxlendPairDeployer_deploySecond. This very owner is able to change the currently used time lock contract address with the FraxlendPair.setTimeLock function. A time lock is enforced on the FraxlendPair.changeFee function whenever the protocol fee is adjusted.

However, as the Fraxlend pair owner is able to change the time lock contract address to any other arbitrary (contract) address, it is possible to circumvent this timelock without users knowing. By using a custom smart contract without an enforced time lock, the protocol fee can be changed at any time without a proper time lock.

Proof of Concept

FraxlendPair.sol#L206

/// @notice The ```setTimeLock``` function sets the TIME_LOCK address
/// @param _newAddress the new time lock address
function setTimeLock(address _newAddress) external onlyOwner {
    emit SetTimeLock(TIME_LOCK_ADDRESS, _newAddress);
    TIME_LOCK_ADDRESS = _newAddress;
}

Tools Used

Manual review

Recommended mitigation steps

Currently, the owner COMPTROLLER_ADDRESS address is trustworthy, however, nothing prevents the above-described scenario. To protect users from sudden protocol fee changes, consider using a minimal time lock implementation directly implemented in the contract without trusting any external contract.

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L137 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L141 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L145 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L149

Vulnerability details

Impact

The FraxlendPair contract implements the EIP4626 standard (EIP-4626: Tokenized Vault Standard).

However, according to EIP4626, the below-mentioned functions do not fully adhere to the specs.

Proof of Concept

FraxlendPair.maxDeposit

function maxDeposit(address) external pure returns (uint256) {
  return type(uint128).max;
}

1. MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0
2. MUST return 2 ** 256 - 1 if there is no limit on the maximum amount of assets that may be deposited

FraxlendPair.maxMint

function maxMint(address) external pure returns (uint256) {
  return type(uint128).max;
}

1. MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0
2. MUST return 2 ** 256 - 1 if there is no limit on the maximum amount of shares that may be minted

FraxlendPair.maxWithdraw

function maxWithdraw(address owner) external view returns (uint256) {
    return totalAsset.toAmount(balanceOf(owner), false);
}

1. MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0

FraxlendPair.maxRedeem

function maxRedeem(address owner) external view returns (uint256) {
    return balanceOf(owner);
}

1. MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0

Tools Used

Manual review

Recommended mitigation steps

Consider implementing the EIP4626 standard as close to the specs as possible.

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairDeployer.sol#L205 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairDeployer.sol#L329-L330 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairDeployer.sol#L253

Vulnerability details

Impact

The FraxlendPairDeployer contract used to deploy Fraxlend pairs prevents deploying pairs with the same salt and _configData. This makes it vulnerable to front-running pair deployments and prevents deploying honest Fraxlend pairs.

Proof of Concept

FraxlendPairDeployer._deployFirst

function _deployFirst(
    bytes32 _saltSeed,
    bytes memory _configData,
    bytes memory _immutables,
    uint256 _maxLTV,
    uint256 _liquidationFee,
    uint256 _maturityDate,
    uint256 _penaltyRate,
    bool _isBorrowerWhitelistActive,
    bool _isLenderWhitelistActive
) private returns (address _pairAddress) {
    {
        // _saltSeed is the same for all public pairs so duplicates cannot be created
        bytes32 salt = keccak256(abi.encodePacked(_saltSeed, _configData));
        require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed"); // @audit-info Reverts if a pair with the same salt is already deployed

        bytes memory _creationCode = BytesLib.concat(
            SSTORE2.read(contractAddress1),
            SSTORE2.read(contractAddress2)
        );
        bytes memory bytecode = abi.encodePacked(
            _creationCode,
            abi.encode(
                _configData,
                _immutables,
                _maxLTV,
                _liquidationFee,
                _maturityDate,
                _penaltyRate,
                _isBorrowerWhitelistActive,
                _isLenderWhitelistActive
            )
        );

        assembly {
            _pairAddress := create2(0, add(bytecode, 32), mload(bytecode), salt)
        }
        require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");

        deployedPairsBySalt[salt] = _pairAddress;
    }

    return _pairAddress;
}

A deployed Fraxlend pair address is stored in the deployedPairsBySalt mapping using a salt as the key. This salt is generated based on a seed _saltSeed and _configData. Deploying a standard Fraxlend pair with FraxlendPairDeployer.deploy uses _saltSeed = keccak256(abi.encodePacked("public")) and user-defined _configData. The salt is the same for all standard deployments.

FraxlendPairDeployer.deploy

function deploy(bytes memory _configData) external returns (address _pairAddress) {
    (address _asset, address _collateral, , , , address _rateContract, ) = abi.decode(
        _configData,
        (address, address, address, address, uint256, address, bytes)
    );
    string memory _name = string(
        abi.encodePacked(
            "FraxlendV1 - ",
            IERC20(_collateral).safeName(),
            "/",
            IERC20(_asset).safeName(),
            " - ",
            IRateCalculator(_rateContract).name(),
            " - ",
            (deployedPairsArray.length + 1).toString()
        )
    );

    _pairAddress = _deployFirst(
        keccak256(abi.encodePacked("public")),
        _configData,
        abi.encode(CIRCUIT_BREAKER_ADDRESS, COMPTROLLER_ADDRESS, TIME_LOCK_ADDRESS, FRAXLEND_WHITELIST_ADDRESS),
        DEFAULT_MAX_LTV,
        DEFAULT_LIQ_FEE,
        0,
        0,
        false,
        false
    );

    _deploySecond(_name, _pairAddress, _configData, new address[](0), new address[](0));

    _logDeploy(_name, _pairAddress, _configData, DEFAULT_MAX_LTV, DEFAULT_LIQ_FEE, 0);
}

A whitelisted (and potentially dishonest) user could watch the mempool for new Fraxlend pair deployments and front-run deployments with a custom pair deployment by calling FraxlendPairDeployer.deployCustom.

FraxlendPairDeployer.deployCustom

function deployCustom(
    string memory _name,
    bytes memory _configData,
    uint256 _maxLTV,
    uint256 _liquidationFee,
    uint256 _maturityDate,
    uint256 _penaltyRate,
    address[] memory _approvedBorrowers,
    address[] memory _approvedLenders
) external returns (address _pairAddress) {
    require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
    require(
        IFraxlendWhitelist(FRAXLEND_WHITELIST_ADDRESS).fraxlendDeployerWhitelist(msg.sender),
        "FraxlendPairDeployer: Only whitelisted addresses"
    );

    _pairAddress = _deployFirst(
        keccak256(abi.encodePacked(_name)),
        _configData,
        abi.encode(CIRCUIT_BREAKER_ADDRESS, COMPTROLLER_ADDRESS, TIME_LOCK_ADDRESS, FRAXLEND_WHITELIST_ADDRESS),
        _maxLTV,
        _liquidationFee,
        _maturityDate,
        _penaltyRate,
        _approvedBorrowers.length > 0,
        _approvedLenders.length > 0
    );

    _deploySecond(_name, _pairAddress, _configData, _approvedBorrowers, _approvedLenders);

    deployedPairCustomStatusByAddress[_pairAddress] = true;

    _logDeploy(_name, _pairAddress, _configData, _maxLTV, _liquidationFee, _maturityDate);
}

Internally the FraxlendPairDeployer.deployCustom uses the same _deployFirst function as a standard pair deployment uses. However, the salt is user-definable by using any _name, for instance public. Then the salt is the same as _saltSeed = keccak256(abi.encodePacked("public")). The user then uses the same _configData as the to be deployed pair. But, contrary to a standard pair deployment, a whitelisted user is also able to provide additional configuration, _maxLTV, _liquidationFee and so on.

The whitelisted user is able to deploy a custom Fraxlend pair with the same salt and the same _configData but by using for instance a _maturityDate < block.timestamp, this deployed custom pool is unusable (due to the modifier isNotPastMaturity reverting. This modifier is used by FraxlendPairCore.deposit, FraxlendPairCore.mint, FraxlendPairCore.borrowAsset, FraxlendPairCore.addCollateral and FraxlendPairCore.leveragedPosition).

As FraxlendPairDeployer._deployFirst checks if a pair is already deployed with a given salt, the honest pair deployment that got front-run reverts. It is now not possible to deploy a pair with the same configuration.

NOTE: The same can be achieved by front-running a pair deployment transaction with a custom pair deployment and with the same name (FraxlendPairDeployer.deployCustom allows providing an arbitrary name . FraxlendPairDeployer._deploySecond checks the name of the pair and reverts if a pair with that name already exists:

FraxlendPairDeployer._deploySecond

function _deploySecond(
    string memory _name,
    address _pairAddress,
    bytes memory _configData,
    address[] memory _approvedBorrowers,
    address[] memory _approvedLenders
) private {
    (, , , , , , bytes memory _rateInitData) = abi.decode(
        _configData,
        (address, address, address, address, uint256, address, bytes)
    );
    require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique"); // @audit-info reverts if a pair with the same name exists already
    deployedPairsByName[_name] = _pairAddress;
    deployedPairsArray.push(_name);

    // Set additional values for FraxlendPair
    IFraxlendPair _fraxlendPair = IFraxlendPair(_pairAddress);
    _fraxlendPair.initialize(_name, _approvedBorrowers, _approvedLenders, _rateInitData);

    // Transfer Ownership of FraxlendPair
    _fraxlendPair.transferOwnership(COMPTROLLER_ADDRESS);
}

Tools Used

Manual review

Recommended mitigation steps

Consider validating the _name parameter in the FraxlendPairDeployer.deployCustom function to not equal the string public, thus preventing the usage of the same salt as a standard pair deployment.

Additionally, consider prefixing the name of a custom pair deployment to also prevent front-running this way.

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.