Fraxlend (Frax Finance) contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

The specified timelock address in FraxlendPair.sol serves the sole purpose of providing a timelock for updating the feeToProtocolRate. This variable controls the amount of interest that is directed to the protocol instead of the lenders with a maximum fee of 50% (per the comments). Since the timelock contract can be arbitrarily set without a timelock of its own, the fee can be changed without a timelock by extension.

Proof of Concept

The changeFee() function requires that the caller is the specified timelock contract:

    function changeFee(uint32 _newFee) external whenNotPaused {
        if (msg.sender != TIME_LOCK_ADDRESS) revert OnlyTimeLock();
        ...
    }

However, the TIME_LOCK_ADDRESS can be set at any time by the owner:

    function setTimeLock(address _newAddress) external onlyOwner {
        emit SetTimeLock(TIME_LOCK_ADDRESS, _newAddress);
        TIME_LOCK_ADDRESS = _newAddress;
    }

Tools Used

Manual review.

Recommended Mitigation Steps

In order to maintain the flexibility of changing the timelock address, I recommend adding the same check to setTimeLock() that requires that the caller is the original timelock contract. In other words, setting a new timelock contract requires a waiting period defined by the original timelock contract.

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Floating Pragma

A floating pragma might result in contract being tested/deployed with different compiler versions possibly leading to unexpected behaviour. 1 instance of this issue has been found:

[L-01] FraxlendPairCore.sol#L2-L3

pragma solidity ^0.8.15;

[L-02] approve has been deprecated

Please use safeIncreaseAllowance and safeDecreaseAllowance instead 4 instances of this issue have been found:

[L-02] FraxlendPairCore.sol#L1184-L1185

        _collateralContract.approve(_swapperAddress, _collateralToSwap);

[L-02b] FraxlendPairCore.sol#L1103-L1104

        _assetContract.approve(_swapperAddress, _borrowAmount);

[L-02c] FraxlendPairCore.sol#L1184-L1185

        _collateralContract.approve(_swapperAddress, _collateralToSwap);

[L-02d] FraxlendPairCore.sol#L633-L634

            if (allowed != type(uint256).max) _approve(_owner, msg.sender, allowed - _shares);

[L-03] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. 1 instance of this issue has been found:

[L-03] FraxlendPairDeployer.sol#L204-L205

            bytes32 salt = keccak256(abi.encodePacked(_saltSeed, _configData));

[L-04] Missing zero address check might lead to redeployment

If variable is accidentally set to zero the contract will have to be redeployed. 1 instance of this issue has been found:

[L-04] FraxlendPairCore.sol#L171-L176

            DEPLOYER_ADDRESS = msg.sender;
            CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
            COMPTROLLER_ADDRESS = _comptrollerAddress;
            TIME_LOCK_ADDRESS = _timeLockAddress;
            FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelistAddress;
        }

Non-critical Findings

[N-01] The use of magic numbers is not recommended

Consider setting constant numbers as a constant variable for better readability and clarity. 3 instances of this issue have been found:

[N-01] FraxlendPairDeployer.sol#L171-L172

        bytes memory _firstHalf = BytesLib.slice(_creationCode, 0, 13000);

[N-01b] FraxlendPairDeployer.sol#L173-L174

        if (_creationCode.length > 13000) {

[N-01c] FraxlendPairDeployer.sol#L174-L175

            bytes memory _secondHalf = BytesLib.slice(_creationCode, 13000, _creationCode.length - 13000);

[N-02] Unindexed parameters in events

Events should index all first three parameters. 4 instances of this issue have been found:

[N-02] FraxlendPairCore.sol#L1143-L1149

    event RepayAssetWithCollateral(
        address indexed _borrower,
        address _swapperAddress,
        uint256 _collateralToSwap,
        uint256 _amountAssetOut,
        uint256 _sharesRepaid
    );

[N-02b] FraxlendPairCore.sol#L1045-L1052

    event LeveragedPosition(
        address indexed _borrower,
        address _swapperAddress,
        uint256 _borrowAmount,
        uint256 _borrowShares,
        uint256 _initialCollateralAmount,
        uint256 _amountCollateralOut
    );

[N-02c] FraxlendWhitelist.sol#L45-L46

    event SetOracleWhitelist(address indexed _address, bool _bool);

[N-02d] FraxlendPairCore.sol#L897-L904

    event Liquidate(
        address indexed _borrower,
        uint256 _collateralForLiquidator,
        uint256 _sharesToLiquidate,
        uint256 _amountLiquidatorToRepay,
        uint256 _sharesToAdjust,
        uint256 _amountToAdjust
    );

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] Variable should be made a constant

A variable that is not changed should be made constant in order to save gas. 3 instances of this issue have been found:

[G-01] FraxlendPairDeployer.sol#L49-L51

    uint256 public DEFAULT_MAX_LTV = 75000; // 75% with 1e5 precision
    uint256 public GLOBAL_MAX_LTV = 1e8; // 1000x (100,000%) with 1e5 precision, protects from rounding errors in LTV calc
    uint256 public DEFAULT_LIQ_FEE = 10000; // 10% with 1e5 precision

[G-01b] FraxlendPairDeployer.sol#L57-L59

    address public CIRCUIT_BREAKER_ADDRESS;
    address public COMPTROLLER_ADDRESS;

[G-01c] FraxlendPairCore.sol#L86-L87

    address public TIME_LOCK_ADDRESS;

[G-02] Using prefix(++i) instead of postfix (i++) saves gas

It saves 6 gas per iteration. 5 instances of this issue have been found:

[G-02] FraxlendWhitelist.sol#L81-L82

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-02b] FraxlendWhitelist.sol#L66-L67

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-02c] FraxlendWhitelist.sol#L51-L52

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-02d] FraxlendPair.sol#L289-L290

        for (uint256 i = 0; i < _lenders.length; i++) {

[G-02e] FraxlendPair.sol#L308-L309

        for (uint256 i = 0; i < _borrowers.length; i++) {

[G-03] Setting variable to default value is redundant

Variables are by default set to 0 and is a waist of gas if variable sits in storage. Examples to avoid: uint i = 0 bool success = false 9 instances of this issue have been found:

[G-03] FraxlendWhitelist.sol#L81-L82

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-03b] FraxlendWhitelist.sol#L66-L67

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-03c] FraxlendWhitelist.sol#L51-L52

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-03d] FraxlendPairDeployer.sol#L127-L128

        for (i = 0; i < _lengthOfArray; ) {

[G-03e] FraxlendPair.sol#L289-L290

        for (uint256 i = 0; i < _lenders.length; i++) {

[G-03f] FraxlendPairDeployer.sol#L402-L403

        for (uint256 i = 0; i < _lengthOfArray; ) {

[G-03g] FraxlendPair.sol#L308-L309

        for (uint256 i = 0; i < _borrowers.length; i++) {

[G-03h] FraxlendPairCore.sol#L270-L271

        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

[G-03i] FraxlendPairCore.sol#L265-L266

        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

[G-04] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

5 instances of this issue have been found:

[G-04] FraxlendWhitelist.sol#L81-L82

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-04b] FraxlendWhitelist.sol#L66-L67

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-04c] FraxlendWhitelist.sol#L51-L52

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-04d] FraxlendPairCore.sol#L270-L271

        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

[G-04e] FraxlendPairCore.sol#L265-L266

        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

[G-05] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

7 instances of this issue have been found:

[G-05] FraxlendWhitelist.sol#L81-L82

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-05b] FraxlendWhitelist.sol#L66-L67

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-05c] FraxlendWhitelist.sol#L51-L52

        for (uint256 i = 0; i < _addresses.length; i++) {

[G-05d] FraxlendPair.sol#L289-L290

        for (uint256 i = 0; i < _lenders.length; i++) {

[G-05e] FraxlendPair.sol#L308-L309

        for (uint256 i = 0; i < _borrowers.length; i++) {

[G-05f] FraxlendPairCore.sol#L270-L271

        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

[G-05g] FraxlendPairCore.sol#L265-L266

        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

[G-06] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operatio 2 instances of this issue have been found:

[G-06] FraxlendPairCore.sol#L132-L137

    // Internal Whitelists
    bool public immutable borrowerWhitelistActive;
    mapping(address => bool) public approvedBorrowers;

    bool public immutable lenderWhitelistActive;
    mapping(address => bool) public approvedLenders;

[G-06b] FraxlendPairCore.sol#L125-L129

    // User Level Accounting
    /// @notice Stores the balance of collateral for each user
    mapping(address => uint256) public userCollateralBalance; // amount of collateral each user is backed
    /// @notice Stores the balance of borrow shares for each user
    mapping(address => uint256) public userBorrowShares

[G-07] Avoid using bool for storage

"Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled." - Source: OZ.

Instead use uint256(1) and uint256(2) for true and false. 5 instances of this issue have been found:

[G-07] FraxlendPairCore.sol#L136-L137

    bool public immutable lenderWhitelistActive;

[G-07b] FraxlendPairCore.sol#L133-L134

    bool public immutable borrowerWhitelistActive;

[G-07c] FraxlendWhitelist.sol#L32-L33

    mapping(address => bool) public oracleContractWhitelist;

[G-07d] FraxlendWhitelist.sol#L35-L36

    mapping(address => bool) public rateContractWhitelist;

[G-07e] FraxlendWhitelist.sol#L38-L39

    mapping(address => bool) public fraxlendDeployerWhitelist;

[G-08] Function could accept array for bool

If function accepted an array for bool it would be more gas efficient because it would be able to set contract for both to true and false in the same call. 3 instances of this issue have been found:

[G-08] FraxlendWhitelist.sol#L80-L85

    function setFraxlendDeployerWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
        for (uint256 i = 0; i < _addresses.length; i++) {
            fraxlendDeployerWhitelist[_addresses[i]] = _bool;
            emit SetFraxlendDeployerWhitelist(_addresses[i], _bool);
        }
    }

[G-08b] FraxlendWhitelist.sol#L65-L70

    function setRateContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
        for (uint256 i = 0; i < _addresses.length; i++) {
            rateContractWhitelist[_addresses[i]] = _bool;
            emit SetRateContractWhitelist(_addresses[i], _bool);
        }
    }

[G-08c] FraxlendWhitelist.sol#L50-L55

    function setOracleContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
        for (uint256 i = 0; i < _addresses.length; i++) {
            oracleContractWhitelist[_addresses[i]] = _bool;
            emit SetOracleWhitelist(_addresses[i], _bool);
        }
    }