Fraxlend (Frax Finance) contest - durianSausage's results

none-critical issue foundings

N01: EVENT IS MISSING INDEXED FIELDS

prof

FraxlendPair.sol, 200, b' event SetTimeLock(address _oldAddress, address _newAddress);' FraxlendPair.sol, 211, b' event ChangeFee(uint32 _newFee);' FraxlendPair.sol, 228, b' event WithdrawFees(uint128 _shares, address _recipient, uint256 _amountToTransfer);' FraxlendPair.sol, 268, b' event SetSwapper(address _swapper, bool _approval);' FraxlendPairCore.sol, 382, b' event AddInterest(\n uint256 _interestEarned,\n uint256 _rate,\n uint256 _deltaTime,\n uint256 _feesAmount,\n uint256 _feesShare\n );' FraxlendPairCore.sol, 389, b' event UpdateRate(uint256 _ratePerSec, uint256 _deltaTime, uint256 _utilizationRate, uint256 _newRatePerSec);' FraxlendPairCore.sol, 504, b' event UpdateExchangeRate(uint256 _rate);'

Low risk

L01: RETURN VALUES OF TRANSFER()/TRANSFERFROM() NOT CHECKED

prof

TRANSFER()/TRANSFERFROM() NOT CHECKED Not all IERC20 implementations revert() when there’s a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

problem

FraxlendPair.sol, 261, b' assetContract.safeTransfer(_recipient, _amountToTransfer);' FraxlendPairCore.sol, 574, b' assetContract.safeTransferFrom(msg.sender, address(this), _amount);' FraxlendPairCore.sol, 651, b' assetContract.safeTransfer(_receiver, _amountToReturn);' FraxlendPairCore.sol, 728, b' assetContract.safeTransfer(_receiver, _borrowAmount);' FraxlendPairCore.sol, 777, b' collateralContract.safeTransferFrom(_sender, address(this), _collateralAmount);' FraxlendPairCore.sol, 819, b' collateralContract.safeTransfer(_receiver, _collateralAmount);' FraxlendPairCore.sol, 872, b' assetContract.safeTransferFrom(_payer, address(this), _amountToRepay);' FraxlendPairDeployer.sol, 262, b' _fraxlendPair.transferOwnership(COMPTROLLER_ADDRESS);' libraries/SafeERC20.sol, 65, b' OZSafeERC20.safeTransfer(token, to, value);' libraries/SafeERC20.sol, 74, b' OZSafeERC20.safeTransferFrom(token, from, to, value);'

L02: _safeMint() should be used rather than _mint() wherever possible

problem

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

prof

FraxlendPairCore.sol, 487, b' _mint(address(this), _feesShare);' FraxlendPairCore.sol, 570, b' _mint(_receiver, _shares);'

L03: Return values of approve() not checked

problem

Not all IERC20 implementations revert() when there’s a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

prof

FraxlendPairCore.sol, 1103, b' _assetContract.approve(_swapperAddress, _borrowAmount);' FraxlendPairCore.sol, 1184, b' _collateralContract.approve(_swapperAddress, _collateralToSwap);'

L03: INPUT ARRAY LENGTHS MAY DIFFER

problem

If the caller makes a copy-paste error, the lengths may be mismatchd and an operation believed to have been completed may not in fact have been completed function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {

prof

FraxlendPairCore.sol, 285, b' function initialize(\n string calldata _name,\n address[] calldata _approvedBorrowers,\n address[] calldata _approvedLenders,\n bytes calldata _rateInitCallData\n ) external ' FraxlendPairDeployer.sol, 388, b' function deployCustom(\n string memory _name,\n bytes memory _configData,\n uint256 _maxLTV,\n uint256 _liquidationFee,\n uint256 _maturityDate,\n uint256 _penaltyRate,\n address[] memory _approvedBorrowers,\n address[] memory _approvedLenders\n ) external returns (address _pairAddress) '

gas optimization

G01: custom error save more gas

problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.

prof

FraxlendPairDeployer.sol, 205, b' require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");' FraxlendPairDeployer.sol, 228, b' require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");' FraxlendPairDeployer.sol, 253, b' require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");' FraxlendPairDeployer.sol, 365, b' require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");' FraxlendPairDeployer.sol, 366, b' require(' FraxlendPairDeployer.sol, 399, b' require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");' LinearInterestRate.sol, 57, b' require(' LinearInterestRate.sol, 61, b' require(' LinearInterestRate.sol, 65, b' require('

G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

FraxlendPairCore.sol, 477, b' if (_currentRateInfo.feeToProtocolRate > 0) {' FraxlendPairCore.sol, 754, b' if (_collateralAmount > 0) {' FraxlendPairCore.sol, 835, b' if (userBorrowShares[msg.sender] > 0) {' FraxlendPairCore.sol, 1002, b' if (_leftoverBorrowShares > 0) {' FraxlendPairCore.sol, 1094, b' if (_initialCollateralAmount > 0) {' LinearInterestRate.sol, 66, b' _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,'

G03: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

FraxlendPair.sol, 289, b' for (uint256 i = 0; i < _lenders.length; i++) {' FraxlendPair.sol, 308, b' for (uint256 i = 0; i < _borrowers.length; i++) {' FraxlendPairDeployer.sol, 130, b' i++;' FraxlendPairDeployer.sol, 158, b' i++;' FraxlendWhitelist.sol, 51, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 66, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 81, b' for (uint256 i = 0; i < _addresses.length; i++) {' libraries/SafeERC20.sol, 24, b' i++;' libraries/SafeERC20.sol, 27, b' for (i = 0; i < 32 && data[i] != 0; i++) {'

G04: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

FraxlendPairCore.sol, 773, b' totalCollateral += _collateralAmount;' FraxlendPairCore.sol, 815, b' totalCollateral -= _collateralAmount;'

G05: USING BOOLS FOR STORAGE INCURS OVERHEAD

problem

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

prof

FraxlendPairCore.sol, 78, b' mapping(address => bool) public swappers; // approved swapper addresses' FraxlendPairCore.sol, 134, b' mapping(address => bool) public approvedBorrowers;' FraxlendPairCore.sol, 137, b' mapping(address => bool) public approvedLenders;' FraxlendPairDeployer.sol, 94, b' mapping(address => bool) public deployedPairCustomStatusByAddress;' FraxlendWhitelist.sol, 32, b' mapping(address => bool) public oracleContractWhitelist;' FraxlendWhitelist.sol, 35, b' mapping(address => bool) public rateContractWhitelist;' FraxlendWhitelist.sol, 38, b' mapping(address => bool) public fraxlendDeployerWhitelist;'

G06: resign the default value to the variables.

problem

resign the default value to the variables will cost more gas.

prof

FraxlendPair.sol, 289, b' for (uint256 i = 0; i < _lenders.length; i++) {' FraxlendPair.sol, 308, b' for (uint256 i = 0; i < _borrowers.length; i++) {' FraxlendPairCore.sol, 265, b' for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {' FraxlendPairCore.sol, 270, b' for (uint256 i = 0; i < _approvedLenders.length; ++i) {' FraxlendPairDeployer.sol, 402, b' for (uint256 i = 0; i < _lengthOfArray; ) {' FraxlendWhitelist.sol, 51, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 66, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 81, b' for (uint256 i = 0; i < _addresses.length; i++) {' libraries/SafeERC20.sol, 22, b' uint8 i = 0;'

G07: ++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

prof

FraxlendPair.sol, 289, b' for (uint256 i = 0; i < _lenders.length; i++) {' FraxlendPair.sol, 308, b' for (uint256 i = 0; i < _borrowers.length; i++) {' FraxlendPairCore.sol, 265, b' for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {' FraxlendPairCore.sol, 270, b' for (uint256 i = 0; i < _approvedLenders.length; ++i) {' FraxlendPairDeployer.sol, 130, b' i++;' FraxlendPairDeployer.sol, 158, b' i++;' FraxlendWhitelist.sol, 51, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 66, b' for (uint256 i = 0; i < _addresses.length; i++) {' FraxlendWhitelist.sol, 81, b' for (uint256 i = 0; i < _addresses.length; i++) {' libraries/SafeERC20.sol, 24, b' i++;' libraries/SafeERC20.sol, 27, b' for (i = 0; i < 32 && data[i] != 0; i++) {'

G08: USE A MORE RECENT VERSION OF SOLIDITY

prof:

Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

G09: Splitting require() statements that use && saves gas

problem

See this issue(https://github.com/code-423n4/2022-01-xdefi-findings/issues/128) which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

prof:

LinearInterestRate.sol, 60, b' require(\n _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,\n "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"\n );' LinearInterestRate.sol, 64, b' require(\n _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,\n "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"\n );' LinearInterestRate.sol, 68, b' require(\n _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,\n "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"\n );'

G10：ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED()

prof

FraxlendPairCore.sol, 450, b' bytes memory _rateData = abi.encode(' FraxlendPairDeployer.sol, 213, b' abi.encode(' LinearInterestRate.sol, 47, b' return abi.encode(MIN_INT, MAX_INT, MAX_VERTEX_UTIL, UTIL_PREC);' VariableInterestRate.sol, 53, b' return abi.encode(MIN_UTIL, MAX_UTIL, UTIL_PREC, MIN_INT, MAX_INT, INT_HALF_LIFE);'

G11: FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

problem

• 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

prof

raxlendPair.sol, 146, b' function maxWithdraw(address owner) external view returns (uint256) ' FraxlendPair.sol, 150, b' function maxRedeem(address owner) external view returns (uint256) ' FraxlendPair.sol, 207, b' function setTimeLock(address _newAddress) external onlyOwner ' FraxlendPair.sol, 263, b' function withdrawFees(uint128 _shares, address _recipient) external onlyOwner returns (uint256 _amountToTransfer) ' FraxlendPair.sol, 277, b' function setSwapper(address _swapper, bool _approval) external onlyOwner ' FraxlendPair.sol, 328, b' function pause() external ' FraxlendPair.sol, 337, b' function unpause() external ' FraxlendPairCore.sol, 653, b' function _redeem(\n VaultAccount memory _totalAsset,\n uint128 _amountToReturn,\n uint128 _shares,\n address _receiver,\n address _owner\n ) internal ' FraxlendPairCore.sol, 669, b' function redeem(\n uint256 _shares,\n address _receiver,\n address _owner\n ) external nonReentrant returns (uint256 _amountToReturn) ' FraxlendPairCore.sol, 685, b' function withdraw(\n uint256 _amount,\n address _receiver,\n address _owner\n ) external nonReentrant returns (uint256 _shares) ' FraxlendPairDeployer.sol, 177, b' function setCreationCode(bytes calldata _creationCode) external onlyOwner ' FraxlendPairDeployer.sol, 263, b' function _deploySecond(\n string memory _name,\n address _pairAddress,\n bytes memory _configData,\n address[] memory _approvedBorrowers,\n address[] memory _approvedLenders\n ) private ' FraxlendWhitelist.sol, 55, b' function setOracleContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner ' FraxlendWhitelist.sol, 70, b' function setRateContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner ' FraxlendWhitelist.sol, 85, b' function setFraxlendDeployerWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner '

G12: USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVES GAS

problem

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table

prof

FraxlendPairCore.sol, 59, b' IERC20 public immutable collateralContract;' FraxlendPairCore.sol, 62, b' address public immutable oracleMultiply;' FraxlendPairCore.sol, 63, b' address public immutable oracleDivide;' FraxlendPairCore.sol, 64, b' uint256 public immutable oracleNormalization;' FraxlendPairCore.sol, 67, b' uint256 public immutable maxLTV;' FraxlendPairCore.sol, 70, b' uint256 public immutable cleanLiquidationFee;' FraxlendPairCore.sol, 71, b' uint256 public immutable dirtyLiquidationFee;' FraxlendPairCore.sol, 74, b' IRateCalculator public immutable rateContract; // For complex rate calculations' FraxlendPairCore.sol, 81, b' address public immutable DEPLOYER_ADDRESS;' FraxlendPairCore.sol, 84, b' address public immutable CIRCUIT_BREAKER_ADDRESS;' FraxlendPairCore.sol, 85, b' address public immutable COMPTROLLER_ADDRESS;' FraxlendPairCore.sol, 89, b' address public immutable FRAXLEND_WHITELIST_ADDRESS;' FraxlendPairCore.sol, 95, b' uint256 public immutable maturityDate;' FraxlendPairCore.sol, 96, b' uint256 public immutable penaltyRate;' FraxlendPairCore.sol, 133, b' bool public immutable borrowerWhitelistActive;' FraxlendPairCore.sol, 136, b' bool public immutable lenderWhitelistActive;'