Ethos Reserve contest - 0xAgro's results

A CDP-backed stablecoin platform designed to generate yield on underlying assets to establish a sustainable DeFi stable interest rate.

General Information

Platform: Code4rena

Start Date: 16/02/2023

Pot Size: $144,750 USDC

Total HM: 17

Participants: 154

Period: 19 days

Judge: Trust

Total Solo HM: 5

Id: 216

League: ETH

Ethos Reserve

Findings Distribution

Researcher Performance

Rank: 29/154

Findings: 1

Award: $455.47

QA:
grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

QA Report

Finding Summary

Low Severity

  1. Unchecked Cast May Overflow
  2. Consider a More Recent Solidity Version
  3. Code Function Does Not Match Comment
  4. Low Coverage
  5. assert Used Over require
  6. Drastic Compiler Version Variation
  7. ERC20 Decimals Assumption
  8. batchLiquidateTroves Ignores Checks In liquidate
  9. LUSD Minting To Restricted Address
  10. No Rebasing Token Support

Non-Critical

  1. Use of ecrecover
  2. Consider Avoiding Sensitive Terms
  3. Explicit Variable Not Used
  4. Variable Scopes Not Explicit
  5. chainid Non-Assembly Function Available
  6. Use of deprecated now
  7. bytes.concat() Can Be Used Over abi.encodePacked()
  8. Contracts Lacking NatSpec Headers
  9. Odd Name State
  10. Be Explicitly Honest About Risk

General Style

  1. Underscore Notation Not Used or Not Used Consistently
  2. Power of Ten Literal Not In Scientific Notation
  3. Use of Exponentiation Over Scientific Notation
  4. Inconsistent Named Returns
  5. Spelling Mistakes
  6. Commented Code
  7. Inconsistent Comment Capitalization
  8. Inconsistent Trailing Period
  9. Needless Comments For Devs
  10. Time Keyword Not Used
  11. Inconsistent Brace Style
  12. Inconsistent Quote Style In Comments
  13. Space Between License and Pragma
  14. Extra Spacing
  15. Header Inconsistency
  16. Inconsistent Function Declaration Style
  17. Variable Names Too General
  18. Functions Lacking NatSpec
  19. State Lacking NatSpec
  20. Inconsistent Exponentiation Style
  21. Differing Ownable Style
  22. Return Variables With Return Statements
  23. Overflow Used For Max Int Type

Style Guide Violations

  1. Maximum Line Length
  2. Order of Functions
  3. Whitespace in Expressions
  4. Control Structures structs
  5. Control Structures if else if else
  6. Function Declaration Style Short
  7. Function Declaration Style Long
  8. Function Declaration Modifier Order
  9. Mappings
  10. Single Quote String
  11. Order of Layout
  12. NatSpec

Low Severity

1. Unchecked Cast May Overflow

As of Solidity 0.8 overflows are handled automatically; however, not for casting. Also, many files in the codebase use Solidity 0.6.11 which does not have overflow protection. For example uint32(4294967300) will result in 4 without reversion. Consider using OpenZepplin's SafeCast library.

/Ethos-Core/contracts/TroveManager.sol

1329:	index = uint128(TroveOwners[_collateral].length.sub(1));

/Ethos-Core/contracts/ActivePool.sol

277:	vars.netAssetMovement = int256(vars.toDeposit) - int256(vars.toWithdraw) - int256(vars.profit);

/Ethos-Vault/contracts/ReaperVaultV2.sol

228:	return -int256(strategies[stratAddr].allocated);
235:	return -int256(stratCurrentAllocation - stratMaxAllocation);
248:	return int256(available);

/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol

121:	roi = -int256(debt - amountFreed);
123:	roi = int256(amountFreed - debt);

/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol

134:	roi = int256(profit);
136:	roi = -int256(allocated - totalAssets);
141:	roi -= int256(loss);

2. Consider a More Recent Solidity Version

Ethos Reserve uses a fork of the Liquity Protocol which uses a solidity version of 0.6.11. It may be better for security to use battle-tested software; however, underflow and overflow oversights can potentially be a large problem, which is fixed post Solidity 0.8.0 (automatic reversion). Consider consulting a Liquity dev to help upgrade past Solidity version 0.8.0 or upgrade yourself depending on your confidence level with the system.

3. Code Function Does Not Match Comment

There is a comment in TroveManager.sol that reads // break if the loop reaches a Trove with ICR >= MCR. The comment is not exactly true. The else block that is commented is triggered if vars.backToNormalMode AND !vars.backToNormalMode OR vars.ICR >= vars.collMCR. Over simplifying comments may lead to users making uninformed decisions when reading the code functionality.

In BorrowerOperations.sol there is a require statement that reads Max fee percentage must less than or equal to 100% which is checked if the system is in recovery mode. If the system is not in recovery another require statement reads Max fee percentage must be between 0.5% and 100%. Both statements check if the max fee percentage falls within inclusive ranges, yet the second require message uses the partially ambiguous (usually said for non-inclusivity) word between. Consider changing this error message from Max fee percentage must be between 0.5% and 100% to Max fee percentage must be between 0.5% and 100% inclusively.

4. Low Coverage

The code coverage of the files in scope is not 100%. Consider a test coverage of 100%.

5. assert Used Over require

assert should only be used in tests. Consider changing all occurrences of assert to require. Prior to Solidity 0.8.0 require will refund all remaining gas whereas assert will not. Even after Solidity 0.8 assert will result in a panic which should not occur in production code. As stated in the Solidity Documentation: "[p]roperly functioning code should never create a Panic".

/Ethos-Core/contracts/BorrowerOperations.sol

128:	assert(MIN_NET_DEBT > 0);
197:	assert(vars.compositeDebt > 0);
301:	assert(msg.sender == _borrower || (msg.sender == stabilityPoolAddress && _collTopUp > 0 && _LUSDChange == 0));
331:	assert(_collWithdrawal <= vars.coll);

/Ethos-Core/contracts/TroveManager.sol

417:	assert(_LUSDInStabPool != 0);
1224:	assert(totalStakesSnapshot[_collateral] > 0);
1279:	assert(closedStatus != Status.nonExistent && closedStatus != Status.active);
1342:	assert(troveStatus != Status.nonExistent && troveStatus != Status.active);
1348:	assert(index <= idxLast);
1414:	assert(newBaseRate > 0);
1489:	assert(decayedBaseRate <= DECIMAL_PRECISION);

/Ethos-Core/contracts/StabilityPool.sol

526:	assert(_debtToOffset <= _totalLUSDDeposits);
551:	assert(_LUSDLossPerUnitStaked <= DECIMAL_PRECISION);
591:	assert(newP > 0);

/Ethos-Core/contracts/LUSDToken.sol

312:	assert(sender != address(0));
313:	assert(recipient != address(0));
321:	assert(account != address(0));
329:	assert(account != address(0));
337:	assert(owner != address(0));
338:	assert(spender != address(0));

6. Drastic Compiler Version Variation

Ethos-Core and Ethos-Vault have drastically different compiler versions. One uses 0.6.11 (which is forked from Liquity) and the other uses ^0.8.0. One major difference between the compiler versions is automatic overflow and underflow checks. In future development or third-party development of the system the two parts are going to need different attention to detail that may introduce bugs. It is best to keep the system as a whole on a similar compiler version.

7. ERC20 Decimals Assumption

EIP20 states in regard to the decimals method: "OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present". The protocol assumes the decimals method is present which may affect the system if whole tokens are used.

/Ethos-Core/contracts/CollateralConfig.sol

uint256 decimals = IERC20(collateral).decimals();

8. batchLiquidateTroves Ignores Checks In liquidate

This issue is from a previous audit of the Liquity Protocol that has not been fixed in Ethos

The liquidate function of TroveManager.sol checks if a trove is active while batchLiquidateTroves does not. Both are publically callable functions.

Please see finding ID: CI-LQY-02 in the 2021 Coinspect Security Audit for more information.

9. LUSD Minting To Restricted Address

This issue is from a previous audit of the Liquity Protocol that has not been fixed in Ethos

When transferring LUSD there is a check done (_requireValidRecipient) to make sure tokens cannot be sent to an invalid address. The mint is missing this check.

Please see finding ID: TOB-LQT-003 in the 2021 Liquity Protocol Trail of Bits Audit for more information.

10. No Rebasing Token Support

In the protocol overview video at time 2:40, Justin states, "liquity is designed to only support ETH, and we support anything".

Rebasing tokens are not checked for (ex). Balances must be adjusted for during transfers.

Please see this issue utilizing similar code from a previous code4rena audit.

Non-Critical

1. Use of ecrecover

Consider using OpenZeppelin's ECDSA contract instead of raw ecrecover to avoid signature malleability.

/Ethos-Core/contracts/LUSDToken.sol

287:	address recoveredAddress = ecrecover(digest, v, r, s);

The 2022 Topshelf Protocol PeckShield Audit which uses the Liquity Protocol code also suggests checking the owner is not the zero address. See finding ID: PVE-001.

2. Consider Avoiding Sensitive Terms

Consider using the terms allowlist / denylist instead of whitelist / blacklist.

/Ethos-Core/contracts/CollateralConfig.sol

37:	event CollateralWhitelisted(address _collateral, uint256 _decimals, uint256 _MCR, uint256 _CCR);
72:	emit CollateralWhitelisted(collateral, decimals, _MCRs[i], _CCRs[i]);

3. Explicit Variable Not Used

As described in the Solidity documentation, "uint and int are aliases for uint256 and int256, respectively". There are moments in the codebase where uint / int is used instead of the explicit uint256 / int256. It is best to be explicit with variable names to lessen confusion. Consider replacing instances of uint / int with uint256 / int256.

/Ethos-Core/contracts/BorrowerOperations.sol

/Ethos-Core/contracts/TroveManager.sol

/Ethos-Core/contracts/ActivePool.sol

/Ethos-Core/contracts/StabilityPool.sol

/Ethos-Core/contracts/LQTY/CommunityIssuance.sol

/Ethos-Core/contracts/LQTY/LQTYStaking.sol

/Ethos-Core/contracts/LUSDToken.sol

4. Variable Scopes Not Explicit

There is an inconsistency with the explicitness of variable scopes. Variable scopes should be explicit and not left out if public (the default). Most variables are explicitly scoped (ex1, ex2, ...); however, not all. The following are not explicitly deemed public:

/Ethos-Core/contracts/BorrowerOperations.sol

/Ethos-Core/contracts/TroveManager.sol

5. chainid Non-Assembly Function Available

If the Solidity version of LUSDToken.sol is upgraded to 0.8.0, the complexity brought by assembly can be avoided by using built in functions. assembly { chainID := chainid() } can be replaced by uint256 chainID = block.chainid.

/Ethos-Core/contracts/LUSDToken.sol

300:	chainID := chainid()

6. Use of deprecated now

now is deprecated, consider using block.timestamp (as used in other parts of the codebase).

/Ethos-Core/contracts/LUSDToken.sol

282:	require(deadline >= now, 'LUSD: expired deadline');

7. bytes.concat() Can Be Used Over abi.encodePacked()

If the contracts are upgraded to Solidity version >= 0.8.4 consider using bytes.concat() instead of abi.encodePacked().

/Ethos-Core/contracts/LUSDToken.sol

283:	bytes32 digest = keccak256(abi.encodePacked('\x19\x01',

8. Contracts Lacking NatSpec Headers

No contracts in scope have a fully annotated NatSpec contract header (@title, @author, etc. see here for reference). Some contracts have a partial NatSpec header like this, some have none, and others have non-NatSpec header comments. Consider adding properly annotated NatSpec headers.

9. Odd Name State

There is a NAME state variable in BorrowerOperations.sol, ActivePool.sol, LQTYStaking.sol, and CommunityIssuance.sol that doesn't seem needed. The same NAME state variable is commented out of TroveManager.sol.

If developers want to know the title of the contract they can reference the source contract name. If there isn't an explicit reason to keep the NAME state variables otherwise, consider removing them.

10. Be Explicitly Honest About Risk

Even if risk seems infinitesimally small it should never be stated as impossible.

For example, in the context of a properly functioning EVM the following line will never overflow (no risk):

uint256 foo = 32;

The next example can overflow in modern Solidity if someArray.length > type(uint32).max even if there are social or intended code concepts keeping the length within a proper length:

uint32 foo = uint32(someArray.length);

L1322-L1223 of TroveManager.sol reads: "No risk of overflow, since troves have minimum LUSD debt of liquidation reserve plus MIN_NET_DEBT. 3e30 LUSD dwarfs the value of all wealth in the world ( which is < 1e15 USD)". This assumes the code keeping the minumum LUSD debt works as described and also assumes the US dollar never experiences extreme hyperinflation. This may be an infinitesimally small risk, but not zero.

Consider changing the line to "Little to no risk of overflow, since troves have minimum LUSD debt of liquidation reserve plus MIN_NET_DEBT. 3e30 LUSD dwarfs the value of all wealth in the world ( which is < 1e15 USD)".

General Style

1. Underscore Notation Not Used or Not Used Consistently

Consider using underscore notation to help with contract readability (Ex. 23453 -> 23_453).

/Ethos-Core/contracts/TroveManager.sol

53:	uint constant public MINUTE_DECAY_FACTOR = 999037758833783000;

2. Power of Ten Literal Not In Scientific Notation

Power of ten literals > 1e2 are easier to read when expressed in scientific notation. Consider expressing large powers of ten in scientific notation.

/Ethos-Core/contracts/TroveManager.sol

54:	uint constant public override REDEMPTION_FEE_FLOOR = DECIMAL_PRECISION / 1000 * 5;

/Ethos-Vault/contracts/ReaperVaultV2.sol

41:	uint256 public constant PERCENT_DIVISOR = 10000;

/Ethos-Core/contracts/ActivePool.sol

145:	require(_treasurySplit + _SPSplit + _stakingSplit == 10_000, "Splits must add up to 10000 BPS");

3. Use of Exponentiation Over Scientific Notation

For better style and less computation consider replacing any power of 10 exponentiation (10**3) with its equivalent scientific notation (1e3).

/Ethos-Vault/contracts/ReaperVaultV2.sol

40:	uint256 public constant DEGRADATION_COEFFICIENT = 10**18;
125:	lockedProfitDegradation = (DEGRADATION_COEFFICIENT * 46) / 10**6;

4. Inconsistent Named Returns

Some functions use named returns and others do not. It is best for code clearity to keep a consistent style.

  1. The following contracts only have named returns (EX. returns(uint256 foo)): CommunityIssuance.sol.
  2. The following contracts only have non-named returns (EX. returns(uint256)): CollateralConfig.sol, and ActivePool.sol.
  3. The following contracts have both: BorrowerOperations.sol, TroveManager.sol, StabilityPool.sol, LQTYStaking.sol, LUSDToken.sol, ReaperVaultV2.sol, ReaperVaultERC4626.sol, ReaperBaseStrategyv4.sol, and ReaperStrategyGranarySupplyOnly.sol.

5. Spelling Mistakes

There are some spelling mistakes throughout the codebase. Consider fixing all spelling mistakes. The following were not included in the automated findings:

/Ethos-Core/contracts/CollateralConfig.sol

  • The word lengths is misspelled as lenghts.

/Ethos-Core/contracts/TroveManager.sol

  • The word elsewhere is misspelled as elswhere.
  • The word borrowers' is misspelled as borrowers's.

6. Commented Code

There are a few commented code lines in TroveManager.sol (L14, L18, L19, L1413). Commented code should be removed prior to production as they no longer serve a purpose and only add bulk or distraction.

7. Inconsistent Comment Capitalization

There is an inconsistency in the capitalization of comments. For example, this line is not capitilized while - like most - this line is. Given almost all comments are capitilized, consider capitilizing all comments.

8. Inconsistent Trailing Period

It appears that the single line comments in the codebase generally follow the following format:

  1. If the comment is a single sentence, then a trailing . is left out (ex1, ex2, ex3, ...)
  2. If the comment has more than one sentence, then a trailing . is added (ex1, ex2, ex3, ...)

There are few, but some comments void this general style:

  1. ex1, ex2, ...

Consider sticking to a single comment style for a less distracting developer experience.

9. Needless Comments For Devs

More documentation is almost always better than none at all; however, there is no need to document basic programming knowledge. Anyone who would want to read the Ethos Reserve code should have basic programming knowledge (knowledge of if / else control statements). There are times in the codebase where basic programming knowledge is commented. In some case like L665, the comment may aid in understanding certain abstraction (a more complex case comparison, although L665 does not tell its full story [see L3]), these are not included. The following comments should be considered for removal in TroveManager.sol: 539, 708, 742.

10. Time Keyword Not Used

Time keywords like hours or days are used in the codebase (ex1, ex2) yet not used here. Consider changing SECONDS_IN_ONE_MINUTE = 60; to SECONDS_IN_ONE_MINUTE = 1 minutes; for consistency.

11. Inconsistent Brace Style

Some single line code does not include braces (ex1, ex2, ...) and others do (ex1, ex2, ex3, ex4, ...). Some single line brace code also have differing spacing (Type1, Type2). In other parts of the codebase single line braces are expanded (ex).

Consider sticking to a single line brace style.

12. Inconsistent Quote Style In Comments

Some comments use a single quote for quotations and others use a double quote. The Solidity Style Guide states that "[s]trings should be quoted with double-quotes instead of single-quotes". Although comments are not seen as active string (most likely does not void the Style Guide), it is best to stick to double quotes for consistency.

13. Space Between License and Pragma

All contracts in scope have a space between the license statement and pragma (ex). Although it does not void the Solidity Style Guide, all example contracts in the Style Guide do not have a space between the license statement and pragma (ex). This format can also be seen throughout the Solidy Documentation (ex). Consider removing the needless space.

14. Extra Spacing

There are times in TroveManager.sol where extra spaces or new lines are present when they are not needed:

  • 476 (double space multi comment line, others are single spaced), 665 (an extra space between else if brace and else), 742 (extra space after else brace and after //), 852 (2 extra new lines), 1323 (space after the opening parenthesis).

15. Header Inconsistency

CommunityIssuance.sol has only basic structural headers (describes basic elements of the contract) which can be removed if _requireCallerIsStabilityPool were converted to a modifier and the contract followed the Solidity Style Guide layout.

Some contracts like ActivePool.sol have meaningful headers that help developers understand the structure of the code. Some contracts like CollateralConfig.sol have no headers at all.

There is no need for CommunityIssuance.sol to have headers if properly formatted. Consider removing the headers or add headers to all contracts.

16. Inconsistent Function Declaration Style

Some function declarations like this do not drop the opening parenthesis on a new line, but some do.

This does not void the Solidity Style Guide which states: "[f]or long function declarations, it is recommended to drop each argument onto its own line at the same indentation level as the function body. The closing parenthesis and opening bracket should be placed on their own line as well at the same indentation level as the function declaration". There is no comment on the placement of the opening parenthesis; however, consider sticking to a single style.

17. Variable Names Too General

There are times in the codebase where a variable of the name vars is used to hold local variables of a function. Although the type of these variables are explicitly named like LocalVariables_openTrove it is best to not use general names like vars as it could confuse the reader. Consider being more specific in naming variables.

18. Functions Lacking NatSpec

In order to understand variables in the codebase the Liquity Documentation was needed. For example, in the openTrove function, the use of the _maxFeePercentage variable was unclear. Consider adding a NatSpec header to all functions which include @param tags detailing the use of each variable.

19. State Lacking NatSpec

Similarily to functions all state variables should be documented with NatSpec tags. To understand what baseRate was, the Liquity Documentation was needed. It is not good to force readers to reference forked code documentation to understand variable use. Consider documenting state varaibles with NatSpec.

20. Inconsistent Exponentiation Style

Non-symmetric spacing voids the Solidity Style Guide; however, it is not clear if exponentiation should have a trailing and leading space or no spacing (a**b vs a ** b). Nevertheless, spacing in exponentiation should be consistant.

In this example the a**b style is used, but in this example the a ** b style is used.

Consider sticking to a single style.

21. Differing Ownable Style

Some contracts in the codebase are ownable; however, they use different styles of ownership. For example BorrowerOperations.sol uses an Ownable contract, whereas TroveManager.sol uses a single variable owner. It is understood that the single variable use in TroveManager.sol is "to circumvent contract size limit". Nonetheless, the same style should be used to avoid possible security confusion and future implementation overlooks.

Also consider using battle-tested ownable contracts like Openzepplin's.

22. Return Variables With Return Statements

Some functions in the codebase use named returns but still return a variable. This can confuse a reader.

/Ethos-Core/contracts/TroveManager.sol

/Ethos-Core/contracts/StabilityPool.sol

/Ethos-Vault/contracts/ReaperVaultERC4626.sol

/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol

23. Overflow Used For Max Int Type

uint256(-1) is used to get the max value of a uint256. Since Solidity 0.6.8 type(uint256).max may be used instead of forcing an overflow. Solidity 0.8.0 introduced automatic overflow detection, forcing non-unchecked instances of uint256(-1) to revert. Future developers may be confused by the use of uint256(-1), consider using type(uint256).max.

/Ethos-Core/contracts/ActivePool.sol

258:	vars.percentOfFinalBal = vars.finalBalance == 0 ? uint256(-1) : vars.currentAllocated.mul(10_000).div(vars.finalBalance);

Style Guide Violations

1. Maximum Line Length

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

The following lines are longer than 120 characters, it is suggested to shorten these lines:

/Ethos-Core/contracts/BorrowerOperations.sol

/Ethos-Core/contracts/TroveManager.sol

/Ethos-Core/contracts/ActivePool.sol

/Ethos-Core/contracts/StabilityPool.sol

/Ethos-Core/contracts/LQTY/LQTYStaking.sol

/Ethos-Core/contracts/LUSDToken.sol

/Ethos-Vault/contracts/ReaperVaultERC4626.sol

2. Order of Functions

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

3. Whitespace in Expressions

The Solidity Style Guide recommends to "[a]void extraneous whitespace [i]mmediately inside parenthesis, brackets or braces, with the exception of single line function declarations".

/Ethos-Core/contracts/StabilityPool.sol

368:	if (_amount !=0) {_requireNoUnderCollateralizedTroves();}
849:	require( msg.sender == address(activePool), "StabilityPool: Caller is not ActivePool");

/Ethos-Core/contracts/TroveManager.sol

1127:	if ( rewardPerUnitStaked == 0 || Troves[_borrower][_collateral].status != Status.active) { return 0; }
1142:	if ( rewardPerUnitStaked == 0 || Troves[_borrower][_collateral].status != Status.active) { return 0; }

4. Control Structures struct

The Solidity Style Guide states that structs should "close on their own line at the same indentation level as the beginning of the declaration". The struct seen here is defined using a single line which voids the second Style Guide requirement.

5. Control Structures if else if else

The Solidity Style Guide states that "[f]or if blocks which have an else or else if clause, the else should be placed on the same line as the if’s closing brace". This is followed perfectly here and in all contracts except TroveManager.sol: 651, 853.

6. Function Declaration Style Short

The Solidity Style Guide states that "[f]or short function declarations, it is recommended for the opening brace of the function body to be kept on the same line as the function declaration". It is not clear what length is exactly meant by "short" or "long". The maximum line length of 120 characters may be an indication, and in that case any expanded function declaration under 120 characters should be on a single line.

The following functions in their respective contracts are not compliant by this standard:

/Ethos-Core/contracts/TroveManager.sol

  • getCurrentICR.

/Ethos-Core/contracts/LQTY/CommunityIssuance.sol

  • setAddresses.

/Ethos-Vault/contracts/ReaperVaultV2.sol

  • addStrategy.

/Ethos-Vault/contracts/ReaperVaultERC4626.sol

  • withdraw, redeem.

/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol

  • updateVeloSwapPath.

7. Function Declaration Style Long

The Solidity Style Guide states that "[f]or long function declarations, it is recommended to drop each argument onto its own line at the same indentation level as the function body. The closing parenthesis and opening bracket should be placed on their own line as well at the same indentation level as the function declaration."

This function violates the description.

8. Function Declaration Modifier Order

The Solidity Style Guide suggests the following modifer order: Visibility, Mutability, Virtual, Override, Custom modifiers.

/Ethos-Core/contracts/CollateralConfig.sol

  • getAllowedCollaterals ('external', 'override', 'view'): view (Mutability) positioned after override (Override).
  • isCollateralAllowed ('external', 'override', 'view'): view (Mutability) positioned after override (Override).
  • getCollateralDecimals ('external', 'override', 'view', 'Custom'): view (Mutability) positioned after override (Override).
  • getCollateralMCR ('external', 'override', 'view', 'Custom'): view (Mutability) positioned after override (Override).
  • getCollateralCCR ('external', 'override', 'view', 'Custom'): view (Mutability) positioned after override (Override).

/Ethos-Core/contracts/BorrowerOperations.sol

  • _getNewNominalICRFromTroveChange ('pure', 'internal'): internal (Visibility) positioned after pure (Mutability).
  • _getNewICRFromTroveChange ('pure', 'internal'): internal (Visibility) positioned after pure (Mutability).

/Ethos-Core/contracts/StabilityPool.sol

  • depositSnapshots_S ('external', 'override', 'view'): view (Mutability) positioned after override (Override).

/Ethos-Core/contracts/LQTY/CommunityIssuance.sol

  • setAddresses ('external', 'onlyOwner', 'override'): override (Override) positioned after onlyOwner (Custom Modifiers).

/Ethos-Core/contracts/LQTY/LQTYStaking.sol

  • setAddresses ('external', 'onlyOwner', 'override'): override (Override) positioned after onlyOwner (Custom Modifiers).

9. Mappings

Mapping whitespace not in accordance with the Solidity Style Guide. There are many voiding whitespaces (see link).

/Ethos-Core/contracts/CollateralConfig.sol

35:	mapping (address => Config) public collateralConfig;

/Ethos-Core/contracts/TroveManager.sol

86:	mapping (address => mapping (address => Trove)) public Troves;
88:	mapping (address => uint) public totalStakes;
91:	mapping (address => uint) public totalStakesSnapshot;
94:	mapping (address => uint) public totalCollateralSnapshot;
104:	mapping (address => uint) public L_Collateral;
105:	mapping (address => uint) public L_LUSDDebt;
109:	mapping (address => mapping (address => RewardSnapshot)) public rewardSnapshots;
116:	mapping (address => address[]) public TroveOwners;
119:	mapping (address => uint) public lastCollateralError_Redistribution;
120:	mapping (address => uint) public lastLUSDDebtError_Redistribution;

/Ethos-Core/contracts/ActivePool.sol

41:	mapping (address => uint256) internal collAmount;
42:	mapping (address => uint256) internal LUSDDebt;
44:	mapping (address => uint256) public yieldingPercentage;
45:	mapping (address => uint256) public yieldingAmount;
46:	mapping (address => address) public yieldGenerator;
47:	mapping (address => uint256) public yieldClaimThreshold;

/Ethos-Core/contracts/StabilityPool.sol

167:	mapping (address => uint256) internal collAmounts;
179:	mapping (address => uint) S;
186:	mapping (address => Deposit) public deposits;
187:	mapping (address => Snapshots) public depositSnapshots;
214:	mapping (uint128 => mapping(uint128 => mapping (address => uint))) public epochToScaleToSum;
223:	mapping (uint128 => mapping(uint128 => uint)) public epochToScaleToG;
228:	mapping (address => uint) public lastCollateralError_Offset;

/Ethos-Core/contracts/LQTY/LQTYStaking.sol

25:	mapping( address => uint) public stakes;
28:	mapping (address => uint) public F_Collateral;
32:	mapping (address => Snapshot) public snapshots;
35:	mapping (address => uint) F_Collateral_Snapshot;

/Ethos-Core/contracts/LUSDToken.sol

54:	mapping (address => uint256) private _nonces;
57:	mapping (address => uint256) private _balances;
58:	mapping (address => mapping (address => uint256)) private _allowances;
62:	mapping (address => bool) public troveManagers;
63:	mapping (address => bool) public stabilityPools;
64:	mapping (address => bool) public borrowerOperations;

10. Single Quote String

The Solidity Style Guide states that "[s]trings should be quoted with double-quotes instead of single-quotes". Consider using double-quotes to be compliant.

/Ethos-Core/contracts/ActivePool.sol

5:	import './Interfaces/IActivePool.sol';
7:	import './Interfaces/IDefaultPool.sol';

/Ethos-Core/contracts/StabilityPool.sol

5:	import './Interfaces/IBorrowerOperations.sol';
7:	import './Interfaces/IStabilityPool.sol';
8:	import './Interfaces/IBorrowerOperations.sol';
9:	import './Interfaces/ITroveManager.sol';
10:	import './Interfaces/ILUSDToken.sol';
11:	import './Interfaces/ISortedTroves.sol';
870:	require(_initialDeposit > 0, 'StabilityPool: User must have a non-zero deposit');
874:	require(_amount > 0, 'StabilityPool: Amount must be non-zero');

/Ethos-Core/contracts/LQTY/LQTYStaking.sol

266:	require(currentStake > 0, 'LQTYStaking: User must have a non-zero stake');
270:	require(_amount > 0, 'LQTYStaking: Amount must be non-zero');

/Ethos-Core/contracts/LUSDToken.sol

280:	revert('LUSD: Invalid s value');
282:	require(deadline >= now, 'LUSD: expired deadline');
283:	bytes32 digest = keccak256(abi.encodePacked('\x19\x01',
283:	bytes32 digest = keccak256(abi.encodePacked('\x19\x01',
288:	require(recoveredAddress == owner, 'LUSD: invalid signature');

11. Order of Layout

The Solidity Style Guide suggests the following contract layout order: Type declarations, State variables, Events, Modifiers, Functions.

The following contracts are not compliant (examples are only to prove the layout are out of order NOT a full description):

12. NatSpec

The NatSpec notation requires the use of /// or /** to differentiate from regular comments. There is an instance in the codebase where a // is used instead of /// and where a /* is used instead of /**.

/Ethos-Core/contracts/LQTY/CommunityIssuance.sol

83:    // @dev issues a set amount of Oath to the stability pool
97:	/*
98:	 @dev funds the contract and updates the distribution
99:	 @param amount: amount of $OATH to send to the contract
100:	*/

#0 - c4-judge

2023-03-10T09:54:44Z

trust1995 marked the issue as grade-a

#1 - c4-sponsor

2023-03-28T21:30:36Z

0xBebis marked the issue as sponsor confirmed

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