Back to reassor's contests

Fraxlend (Frax Finance) contest - reassor's results

Fraxlend: A permissionless lending platform and the final piece of the Frax Finance Defi Trinity.

General Information

Platform: Code4rena

Start Date: 12/08/2022

Pot Size: $50,000 USDC

Total HM: 15

Participants: 120

Period: 5 days

Judge: Justin Goro

Total Solo HM: 6

Id: 153

League: ETH

Frax Finance

Findings Distribution

Researcher Performance

Rank: 16/120

Findings: 3

Award: $326.47

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryFrax Finance

Findings Information

🌟 Selected for report: carlitox477

Also found by: 0xA5DF, 0xDjango, IllIllI, brgltd, reassor

Labels

bug
duplicate
2 (Med Risk)

Awards

249.5468 USDC - $249.55

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Contract FraxlendPair implements logic for changing timelock and protocol fee. The functionality of changing fee is reserved to TIME_LOCK_ADDRESS which is expected to be a timelock contract that protects users from accidental changes of fee while using the protocol. The issue is that owner can use setTimeLock at any time and change TIME_LOCK_ADDRESS to EOA address and execute changeFee to increase the value of fee to maximum.

Scenario:

  1. Users interact with protocol while the fee is 5%.
  2. Users trigger some transactions.
  3. Owner changes timelock address to himself and then changes the fee to 50%.
  4. Owner's transaction is being included before users transactions which ends up with users being charged fee of 50%.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add timelock to setTimeLock.

#0 - 0xA5DF

2022-08-30T19:24:41Z

Dupe of #249

#1 - DrakeEvans

2022-09-06T13:13:30Z

duplicate #129

Findings Information

🌟 Selected for report: 0x1f8b

Also found by: 0x52, 0xA5DF, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, EthLedger, Funen, IllIllI, JC, Junnon, Lambda, LeoS, MiloTruck, Noah3o6, PaludoX0, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, SaharAP, Sm4rty, SooYa, The_GUILD, TomJ, Waze, Yiko, _Adam, __141345__, a12jmx, ak1, asutorufos, auditor0517, ayeslick, ballx, beelzebufo, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, cccz, cryptonue, cryptphi, d3e4, delfin454000, dipp, djxploit, durianSausage, dy, erictee, fatherOfBlocks, gogo, gzeon, hyh, ignacio, kyteg, ladboy233, medikko, mics, minhquanym, oyc_109, pfapostol, rbserver, reassor, ret2basic, robee, sach1r0, simon135, sryysryy, tabish, yac, yash90, zzzitron

Awards

55.7393 USDC - $55.74

Labels

bug
QA (Quality Assurance)

External Links

Link to GitHub issue

List

Low Risk

  1. Missing zero address checks
  2. Critical address change

Non-Critical Risk

  1. Missing indexed events
  2. Usage of not well-tested solidity version might contain undiscovered vulnerabilities
  3. Contracts are using unlocked pragma
  4. Missing/incomplete natspec comments

1. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

FraxlendWhitelist.sol:

FraxlendPairDeployer.sol:

FraxlendPairCore.sol:

FraxlendPair.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

2. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

FraxlendWhitelist.sol:

FraxlendPairDeployer.sol:

FraxlendPairCore.sol:

FraxlendPair.sol:

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for changing ownership.

3. Missing indexed events

Risk

Non-Critical

Impact

Protocol implements multiple events but does not properly index parameters for all of them. Lack of indexed parameters for events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

FraxlendPair.sol:200:    event SetTimeLock(address _oldAddress, address _newAddress);
FraxlendPair.sol:228:    event WithdrawFees(uint128 _shares, address _recipient, uint256 _amountToTransfer);

Tools Used

Manual Review / VSCode

It is recommended to add indexed keyword to listed events parameters.

4. Usage of not well-tested solidity version might contain undiscovered vulnerabilities

Risk

Non-Critical

Impact

Using the latest versions might make contracts susceptible to undiscovered compiler bugs.

Proof of Concept

FraxlendPair.sol:2:pragma solidity ^0.8.15;
FraxlendPairConstants.sol:2:pragma solidity ^0.8.15;
FraxlendPairCore.sol:2:pragma solidity ^0.8.15;
FraxlendPairDeployer.sol:2:pragma solidity ^0.8.15;
FraxlendWhitelist.sol:2:pragma solidity ^0.8.15;
LinearInterestRate.sol:2:pragma solidity ^0.8.15;
VariableInterestRate.sol:2:pragma solidity ^0.8.15;
libraries/VaultAccount.sol:2:pragma solidity ^0.8.15;
libraries/SafeERC20.sol:2:pragma solidity ^0.8.15;

Tools Used

Manual Review / VSCode

It is recommended to use more stable and tested solidity version such as 0.8.10.

5. Contracts are using unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

FraxlendPair.sol:2:pragma solidity ^0.8.15;
FraxlendPairConstants.sol:2:pragma solidity ^0.8.15;
FraxlendPairCore.sol:2:pragma solidity ^0.8.15;
FraxlendPairDeployer.sol:2:pragma solidity ^0.8.15;
FraxlendWhitelist.sol:2:pragma solidity ^0.8.15;
LinearInterestRate.sol:2:pragma solidity ^0.8.15;
VariableInterestRate.sol:2:pragma solidity ^0.8.15;
libraries/VaultAccount.sol:2:pragma solidity ^0.8.15;
libraries/SafeERC20.sol:2:pragma solidity ^0.8.15;

Tools Used

Manual Review / VSCode

Consider locking compiler version, for example pragma solidity 0.8.15.

6. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Multiple contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

libraries/SafeERC20.sol:

libraries/VaultAccount.sol:

FraxlendPairDeployer.sol:

FraxlendPairCore.sol:

FraxlendPair.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing natspec comments.

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0xA5DF, 0xDjango, 0xNazgul, 0xSmartContract, 0xackermann, 0xbepresent, 0xc0ffEE, 0xkatana, 2997ms, Amithuddar, Aymen0909, Bnke0x0, Chinmay, Chom, CodingNameKiki, Deivitto, Diraco, Dravee, ElKu, EthLedger, Fitraldys, Funen, IgnacioB, JC, Junnon, Lambda, LeoS, Metatron, MiloTruck, Noah3o6, NoamYakov, PaludoX0, Randyyy, ReyAdmirado, Rohan16, Rolezn, Ruhum, SaharAP, Sm4rty, SooYa, TomJ, Tomio, Waze, Yiko, _Adam, __141345__, a12jmx, ajtra, ak1, asutorufos, ballx, brgltd, c3phas, cRat1st0s, carlitox477, chrisdior4, d3e4, delfin454000, dharma09, djxploit, durianSausage, erictee, fatherOfBlocks, find_a_bug, flyx, francoHacker, gerdusx, gogo, gzeon, hakerbaya, ignacio, jag, kyteg, ladboy233, ltyu, m_Rassska, medikko, mics, mrpathfindr, newfork01, nxrblsrpr, oyc_109, pfapostol, rbserver, reassor, ret2basic, robee, sach1r0, saian, simon135, sryysryy, zeesaw

Awards

21.1775 USDC - $21.18

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

List

  1. Cache array length outside of loop
  2. Long revert error messages
  3. Use custom errors instead of revert strings to save gas
  4. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1
  5. Obsolete overflow/underflow check
  6. No need to explicitly initialize variables with default values
  7. Use != 0 instead of > 0 for unsigned integer comparison

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

Proof of Concept

FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairCore.sol:265:        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
FraxlendPairCore.sol:270:        for (uint256 i = 0; i < _approvedLenders.length; ++i) {
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {

It is recommended to cache the array length outside of for loop.

2. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

FraxlendPairDeployer.sol:205:            require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");
FraxlendPairDeployer.sol:228:            require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");
FraxlendPairDeployer.sol:253:        require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");
FraxlendPairDeployer.sol:365:        require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
FraxlendPairDeployer.sol:366:        require(
            IFraxlendWhitelist(FRAXLEND_WHITELIST_ADDRESS).fraxlendDeployerWhitelist(msg.sender),
            "FraxlendPairDeployer: Only whitelisted addresses"
        );
LinearInterestRate.sol:52:    function requireValidInitData(bytes calldata _initData) public pure {
LinearInterestRate.sol:57:        require(
            _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
            "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
        );
LinearInterestRate.sol:61:        require(
            _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
            "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
        );
LinearInterestRate.sol:65:        require(
        require(
            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
            "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
        );

Tools Used

Manual Review / VSCode

It is recommended to decrease revert messages to maximum 32 bytes.

3. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

FraxlendPairCore.sol
FraxlendPairDeployer.sol
LinearInterestRate.sol
VariableInterestRate.sol

It is recommended to add custom errors to listed contracts.

4. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

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

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

5. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

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

It is recommended to wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairConstants.sol:47:    uint16 internal constant DEFAULT_PROTOCOL_FEE = 0; // 1e5 precision
FraxlendPairCore.sol:265:        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
FraxlendPairCore.sol:270:        for (uint256 i = 0; i < _approvedLenders.length; ++i) {
FraxlendPairDeployer.sol:402:        for (uint256 i = 0; i < _lengthOfArray; ) {
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {
LinearInterestRate.sol:33:    uint256 private constant MIN_INT = 0; // 0.00% annual rate
libraries/SafeERC20.sol:22:            uint8 i = 0;

It is recommended to remove explicit initializations with default values.

7. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

FraxlendPairCore.sol:477:                if (_currentRateInfo.feeToProtocolRate > 0) {
FraxlendPairCore.sol:754:        if (_collateralAmount > 0) {
FraxlendPairCore.sol:835:        if (userBorrowShares[msg.sender] > 0) {
FraxlendPairCore.sol:1002:                if (_leftoverBorrowShares > 0) {
FraxlendPairCore.sol:1094:        if (_initialCollateralAmount > 0) {
FraxlendPairDeployer.sol:379:            _approvedBorrowers.length > 0,
FraxlendPairDeployer.sol:380:            _approvedLenders.length > 0
LinearInterestRate.sol:66:            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,

It is recommended to use != 0 instead of > 0.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter