Fraxlend (Frax Finance) contest - erictee's results

[L-01] Avoid floating pragmas for non-library contracts.

Impact

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Findings:

2022-08-frax/src/contracts/LinearInterestRate.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendPairConstants.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/VariableInterestRate.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendPairHelper.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/FraxlendPair.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/libraries/VaultAccount.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L2     pragma solidity ^0.8.15;

2022-08-frax/src/contracts/interfaces/IERC4626.sol:L2     pragma solidity >=0.8.15;

2022-08-frax/src/contracts/interfaces/IFraxlendPair.sol:L2     pragma solidity >=0.8.15;

2022-08-frax/src/contracts/interfaces/ISwapper.sol:L2     pragma solidity >=0.8.15;

2022-08-frax/src/contracts/interfaces/IFraxlendWhitelist.sol:L2     pragma solidity >=0.8.15;

2022-08-frax/src/contracts/interfaces/IRateCalculator.sol:L2     pragma solidity >=0.8.15;

[L-02] _safemint() should be used rather than _mint() wherever possible

Impact

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

Findings:

2022-08-frax/src/contracts/FraxlendPairCore.sol:L487                    _mint(address(this), _feesShare);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L570        _mint(_receiver, _shares);

[L-03] Events not emitted for important state changes.

Impact

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings:

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L170             function setCreationCode(bytes calldata _creationCode) external onlyOwner {

[L-04] Missing checks for address(0x0) when assigning values to address state variables

Findings:

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPair.sol#L206 https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L171-L175 https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L190-L191 https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairDeployer.sol#L104-L107

[N-01] approve() and safeApprove() should be replaced with safeIncreaseAllowance() / safeDecreaseAllowance()

Impact

approve() & safeApprove() are deprecated and subject to a known front-running attack. Consider using safeIncreaseAllowance() & safeDecreaseAllowance() instead.

Findings:

2022-08-frax/src/contracts/FraxlendPairCore.sol:L1103                 _assetContract.approve(_swapperAddress, _borrowAmount);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L1184                 _collateralContract.approve(_swapperAddress, _collateralToSwap);

[G-01] abi.encode() is less efficient than abi.encodePacked()

Impact

Consider using abi.encodePacked() instead for efficieny.

Findings:

2022-08-frax/src/contracts/LinearInterestRate.sol:L47        return abi.encode(MIN_INT, MAX_INT, MAX_VERTEX_UTIL, UTIL_PREC);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L450                bytes memory _rateData = abi.encode(

2022-08-frax/src/contracts/VariableInterestRate.sol:L53        return abi.encode(MIN_UTIL, MAX_UTIL, UTIL_PREC, MIN_INT, MAX_INT, INT_HALF_LIFE);

2022-08-frax/src/contracts/FraxlendPairHelper.sol:L201                    abi.encode(

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L213                abi.encode(

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L331            abi.encode(CIRCUIT_BREAKER_ADDRESS, COMPTROLLER_ADDRESS, TIME_LOCK_ADDRESS, FRAXLEND_WHITELIST_ADDRESS),

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L374            abi.encode(CIRCUIT_BREAKER_ADDRESS, COMPTROLLER_ADDRESS, TIME_LOCK_ADDRESS, FRAXLEND_WHITELIST_ADDRESS),

[G-02] Cache Array Length Outside of Loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L51        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L66        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L81        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L265        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L270        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

2022-08-frax/src/contracts/FraxlendPair.sol:L289        for (uint256 i = 0; i < _lenders.length; i++) {

2022-08-frax/src/contracts/FraxlendPair.sol:L308        for (uint256 i = 0; i < _borrowers.length; i++) {

[G-03] Use custom errors rather than revert()/require() string to save gas

Impact

Custom errors are available from solidity version 0.8.4, it saves around 50 gas each time they are hit by avoiding having to allocate and store the revert string.

Findings:

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L205            require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L228            require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L253        require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L365        require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L399        require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");

[G-04] Empty blocks should be removed or emit something

Impact

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L40    constructor() Ownable() {}

2022-08-frax/src/contracts/VariableInterestRate.sol:L57    function requireValidInitData(bytes calldata _initData) external pure {}

[G-05] Functions guaranteed to revert when called by normal users can be declared as payable.

Impact

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L50    function setOracleContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L65    function setRateContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L80    function setFraxlendDeployerWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L170    function setCreationCode(bytes calldata _creationCode) external onlyOwner {

2022-08-frax/src/contracts/FraxlendPair.sol:L204    function setTimeLock(address _newAddress) external onlyOwner {

2022-08-frax/src/contracts/FraxlendPair.sol:L234    function withdrawFees(uint128 _shares, address _recipient) external onlyOwner returns (uint256 _amountToTransfer) {

2022-08-frax/src/contracts/FraxlendPair.sol:L274    function setSwapper(address _swapper, bool _approval) external onlyOwner {

[G-06] ++i costs less gas than i++, especially when it's used in for loops.

Impact

Saves 6 gas per loop.

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L51        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L66        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L81        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L130                i++;

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L158                i++;

2022-08-frax/src/contracts/FraxlendPair.sol:L289        for (uint256 i = 0; i < _lenders.length; i++) {

2022-08-frax/src/contracts/FraxlendPair.sol:L308        for (uint256 i = 0; i < _borrowers.length; i++) {

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L24                i++;

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L27            for (i = 0; i < 32 && data[i] != 0; i++) {

[G-07] Revert message greater than 32 bytes

Impact

Keep revert message lower than or equal to 32 bytes to save gas.

Findings:

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L205            require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L228            require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L253        require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L365        require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");

2022-08-frax/src/contracts/LinearInterestRate.sol:L57         require(

2022-08-frax/src/contracts/LinearInterestRate.sol:L61        require(

2022-08-frax/src/contracts/LinearInterestRate.sol:L65        require(

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L366        require(

[G-08] Explicit initialization with zero not required

Impact

Explicit initialization with zero is not required for variable declaration because uints are 0 by default. Removing this will reduce contract size and save a bit of gas.

Findings:

2022-08-frax/src/contracts/LinearInterestRate.sol:L33    uint256 private constant MIN_INT = 0; // 0.00% annual rate

2022-08-frax/src/contracts/FraxlendPairConstants.sol:L47    uint16 internal constant DEFAULT_PROTOCOL_FEE = 0; // 1e5 precision

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L51        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L66        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L81        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L265        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L270        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L402        for (uint256 i = 0; i < _lengthOfArray; ) {

2022-08-frax/src/contracts/FraxlendPair.sol:L289        for (uint256 i = 0; i < _lenders.length; i++) {

2022-08-frax/src/contracts/FraxlendPair.sol:L308        for (uint256 i = 0; i < _borrowers.length; i++) {

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L22            uint8 i = 0;

[G-09] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops.

Impact

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L51        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L66        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L81        for (uint256 i = 0; i < _addresses.length; i++) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L265        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {

2022-08-frax/src/contracts/FraxlendPairCore.sol:L270        for (uint256 i = 0; i < _approvedLenders.length; ++i) {

2022-08-frax/src/contracts/FraxlendPair.sol:L289        for (uint256 i = 0; i < _lenders.length; i++) {

2022-08-frax/src/contracts/FraxlendPair.sol:L308        for (uint256 i = 0; i < _borrowers.length; i++) {

[G-10] Public functions not called by the contract should be declared external instead

Impact

public functions that are never called by the contract should be declared external to save gas.

Findings:

2022-08-frax/src/contracts/FraxlendPair.sol:L74    function name() public view override(ERC20, IERC20Metadata) returns (string memory) {

2022-08-frax/src/contracts/FraxlendPair.sol:L416    function symbol() public view override(ERC20, IERC20Metadata) returns (string memory) {

2022-08-frax/src/contracts/FraxlendPair.sol:L760    function decimals() public pure override(ERC20, IERC20Metadata) returns (uint8) {

2022-08-frax/src/contracts/FraxlendPair.sol:L1103    function totalSupply() public view override(ERC20, IERC20) returns (uint256) {

2022-08-frax/src/contracts/FraxlendPair.sol:L1452    function totalAssets() public view virtual returns (uint256) {

[G-11] X += Y costs more gas than X = X + Y for state variables.

Impact

Consider changing X += Y to X = X + Y to save gas.

Findings:

2022-08-frax/src/contracts/FraxlendPairCore.sol:L475                _totalBorrow.amount += uint128(_interestEarned);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L476                _totalAsset.amount += uint128(_interestEarned);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L484                    _totalAsset.shares += uint128(_feesShare);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L566        _totalAsset.amount += _amount;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L567        _totalAsset.shares += _shares;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L718        _totalBorrow.amount += _borrowAmount;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L719        _totalBorrow.shares += uint128(_sharesAdded);

2022-08-frax/src/contracts/FraxlendPairCore.sol:L724        userBorrowShares[msg.sender] += _sharesAdded;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L772        userCollateralBalance[_borrower] += _collateralAmount;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L773        totalCollateral += _collateralAmount;

[G-12] Using bools for storage incurs overhead.

Impact

    // 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Findings:

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L32    mapping(address => bool) public oracleContractWhitelist;

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L35    mapping(address => bool) public rateContractWhitelist;

2022-08-frax/src/contracts/FraxlendWhitelist.sol:L38    mapping(address => bool) public fraxlendDeployerWhitelist;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L78    mapping(address => bool) public swappers; // approved swapper addresses

2022-08-frax/src/contracts/FraxlendPairCore.sol:L133    bool public immutable borrowerWhitelistActive;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L134    mapping(address => bool) public approvedBorrowers;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L136    bool public immutable lenderWhitelistActive;

2022-08-frax/src/contracts/FraxlendPairCore.sol:L137    mapping(address => bool) public approvedLenders;

2022-08-frax/src/contracts/FraxlendPairDeployer.sol:L94    mapping(address => bool) public deployedPairCustomStatusByAddress;

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead.

Impact

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed.

Findings:

2022-08-frax/src/contracts/FraxlendPairConstants.sol:L47    uint16 internal constant DEFAULT_PROTOCOL_FEE = 0; // 1e5 precision

2022-08-frax/src/contracts/FraxlendPair.sol:L84    function decimals() public pure override(ERC20, IERC20Metadata) returns (uint8) {

2022-08-frax/src/contracts/FraxlendPair.sol:L166            uint16 _DEFAULT_PROTOCOL_FEE,

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L22            uint8 i = 0;

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L55    function safeDecimals(IERC20 token) internal view returns (uint8) {

2022-08-frax/src/contracts/libraries/SafeERC20.sol:L57        return success && data.length == 32 ? abi.decode(data, (uint8)) : 18;

2022-08-frax/src/contracts/interfaces/IFraxlendPair.sol:L70    function decimals() external pure returns (uint8);

2022-08-frax/src/contracts/interfaces/IFraxlendPair.sol:L90            uint16 _DEFAULT_PROTOCOL_FEE,