Reserve contest - 0xAgro's results

QA Report

Finding Summary

[NC-01] Contracts Missing @title NatSpec Tag

62 out of 110 of the contracts in scope are missing a @title tag. Given that 48 contracts all have a @title tag, consider adding one per the 62 remaining contracts.

AssetRegistry.sol, Broker.sol, Distributor.sol, Component.sol, Trading.sol, RevenueTrader.sol, RayMathNoRounding.sol, ERC20.sol, IAaveIncentivesController.sol, StaticATokenErrors.sol, IStaticATokenLM.sol, IAToken.sol, ReentrancyGuard.sol, OracleLib.sol, ICToken.sol, RTokenAsset.sol, Asset.sol, SelfdestructTransferMock.sol, BadERC20.sol, AaveLendingPoolMock.sol, UnpricedAssetPlugin.sol, GnosisMockReentrant.sol, USDCMock.sol, ERC20Mock.sol, WETH.sol, NontrivialPegCollateral.sol, BrokerV2.sol, AssetRegistryV2.sol, FurnaceV2.sol, DistributorV2.sol, RevenueTraderV2.sol, BackingManagerV2.sol, StRSRV2.sol, MainV2.sol, RTokenV2.sol, BasketHandlerV2.sol, InvalidFiatCollateral.sol, BadCollateralPlugin.sol, ERC1271Mock.sol, GnosisMock.sol, CTokenMock.sol, ComptrollerMock.sol, EasyAuction.sol, ATokenMock.sol, WBTCMock.sol, InvalidBrokerMock.sol, ZeroDecimalMock.sol, MockableCollateral.sol, InvalidATokenFiatCollateralMock.sol, GnosisTrade.sol, Array.sol, Permit.sol, RedemptionBattery.sol, String.sol, StringCallerMock.sol, ArrayCallerMock.sol, FixedCallerMock.sol, IDeployerRegistry.sol, IGnosis.sol, IStRSRVotes.sol, ITrade.sol, and IVersioned.sol are missing a @title tag.

[NC-02] Double Space In Function Definition

In FixedCallerMock.sol most function definitions have double spaces in the parameters: L19, L26, L29, L38, L41, L44, L47, L50, L53, L56, L59, L62, L65, L68, L71, L74, L77, L80, L83, L86, L89, L92, L95, L98, L101, L104, L109, L112, L115, L118, L121, L124, L127, L130, L133, L136. Consider using a single space for better cleanliness.

[NC-03] Style Guide Voiding Whitespace (parenthesis/brackets/braces)

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". There are many instances of a trailing space in the returns parameter.

/contracts/libraries/test/FixedCallerMock.sol Links: 9, 12, 15, 19, 26, 29, 38, 41, 44, 47, 50, 53, 56, 59, 62, 65, 68, 71, 74, 77, 80, 83, 127, 130, 133, 136.

9:	function toFix_(uint256 x) public pure returns (uint192 ) {
12:	function shiftl_toFix_(uint256 x, int8 d) public pure returns (uint192 ) {
15:	function shiftl_toFix_Rnd(uint256 x, int8 d, RoundingMode rnd) public pure returns (uint192 ) {
19:	function divFix_(uint256 x, uint192  y) public pure returns (uint192 ) {
26:	function fixMin_(uint192  x, uint192  y) public pure returns (uint192 ) {
29:	function fixMax_(uint192  x, uint192  y) public pure returns (uint192 ) {
38:	function toUint(uint192  x) public pure returns (uint256 ) {
41:	function toUintRnd(uint192  x, RoundingMode rnd) public pure returns (uint256 ) {
44:	function shiftl(uint192  x, int8 decimals) public pure returns (uint192 ) {
47:	function shiftlRnd(uint192  x, int8 decimals, RoundingMode rnd) public pure returns (uint192 ) {
50:	function plus(uint192  x, uint192  y) public pure returns (uint192 ) {
53:	function plusu(uint192  x, uint256 y) public pure returns (uint192 ) {
56:	function minus(uint192  x, uint192  y) public pure returns (uint192 ) {
59:	function minusu(uint192  x, uint256 y) public pure returns (uint192 ) {
62:	function mul(uint192  x, uint192  y) public pure returns (uint192 ) {
65:	function mulRnd(uint192  x, uint192  y, RoundingMode rnd) public pure returns (uint192 ) {
68:	function mulu(uint192  x, uint256 y) public pure returns (uint192 ) {
71:	function div(uint192  x, uint192  y) public pure returns (uint192 ) {
74:	function divRnd(uint192  x, uint192  y, RoundingMode rnd) public pure returns (uint192 ) {
77:	function divu(uint192  x, uint256 y) public pure returns (uint192 ) {
80:	function divuRnd(uint192  x, uint256 y, RoundingMode rnd) public pure returns (uint192 ) {
83:	function powu(uint192  x, uint48 y) public pure returns (uint192 ) {
127:	function muluDivu(uint192  x, uint256 y, uint256 z) public pure returns (uint192 ) {
130:	function muluDivuRnd(uint192  x, uint256 y, uint256 z, RoundingMode rnd) public pure returns (uint192 ) {
133:	function mulDiv(uint192  x, uint192  y, uint192  z) public pure returns (uint192 ) {
136:	function mulDivRnd(uint192  x, uint192  y, uint192  z, RoundingMode rnd) public pure returns (uint192 ) {

[NC-04] Contract Layout Voids Solidity Docs

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):

• RecollateralizationLib.sol: State is positioned after Functions.
• Component.sol: Modifiers are positioned after Functions.
• IAToken.sol: Events are positioned after Functions.
• ReentrancyGuard.sol: Modifiers are positioned after Functions.
• WETH.sol: State is positioned after Events.
• NontrivialPegCollateral.sol: State is positioned after Functions.
• EasyAuction.sol: Events are Positioned after Events.
• IFacadeRead.sol: State is positioned after Functions.
• IFurnace.sol: Events are positioned after Functions.
• IMain.sol: Events are positioned after Functions.
• ComponentRegistry.sol: State is positioned after Functions

[NC-05] Long Lines (> 120 Characters)

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:

contracts/plugins/aave/IAaveIncentivesController.sol

• 90.

contracts/plugins/aave/StaticATokenLM.sol

• 51, 55, 520.

contracts/plugins/mocks/ChainlinkMock.sol

• 14.

contracts/plugins/mocks/vendor/EasyAuction.sol

• 653, 715, 734, 873.

[NC-06] Underscore Notation Not Used / Not Used Consistently

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

/contracts/p1/BackingManager.sol Links: 33.

33:	uint48 public constant MAX_TRADING_DELAY = 31536000;

/contracts/p1/Broker.sol Links: 24.

24:	uint48 public constant MAX_AUCTION_LENGTH = 604800;

/contracts/p1/Furnace.sol Links: 16.

16:	uint48 public constant MAX_PERIOD = 31536000;

/contracts/p1/StRSR.sol Links: 37, 38.

37:	uint48 public constant MAX_UNSTAKING_DELAY = 31536000;
38:	uint48 public constant MAX_REWARD_PERIOD = 31536000;

/contracts/plugins/mocks/EasyAuction.sol Links: 124.

124:	uint256 public constant FEE_DENOMINATOR = 1000;

/contracts/plugins/trading/GnosisTrade.sol Links: 27.

27:	uint256 public constant FEE_DENOMINATOR = 1000;

/contracts/mixins/Auth.sol Links: 9, 10.

9:	uint48 constant MAX_SHORT_FREEZE = 2592000;
10:	uint48 constant MAX_LONG_FREEZE = 31536000;

[NC-07] Spelling Mistakes / Grammar

There are some spelling mistakes throughout the codebase. Consider fixing all spelling mistakes.

/contracts/p1/BasketHandler.sol

• The word interpreted is misspelled as interepreted (537).

/contracts/p1/StRSRVotes.sol

• The word amount is misspelled as amound (524).

/contracts/plugins/mocks/GnosisMockReentrant.sol

• The word attempts is misspelled as attemts (10).

/contracts/plugins/aave/StaticATokenLM.sol

• The word account is misspelled as accountt (30).

/contracts/plugins/mocks/EasyAuction.sol

• The word initiate is misspelled as intiate (137).
• See [L-02]

/contracts/interfaces/IAsset.sol

• The word implementation is misspelled as implemntation (86).

/contracts/interfaces/IFacadeWrite.sol

• The word interacting is misspelled as interactin (68).

[NC-08] Events Not Indexed

It is best practice to have 3 indexed fields in events. Indexed fields help off-chain tools analyze on-chain activity through filtering these indexed fields.

/contracts/interfaces/IDeployerRegistry.sol Links: 7, 8, 9.

7:	event DeploymentUnregistered(string version, IDeployer deployer);
8:	event DeploymentRegistered(string version, IDeployer deployer);
9:	event LatestChanged(string version, IDeployer deployer);

/contracts/interfaces/IDistributor.sol Links: 28.

28:	event DistributionSet(address dest, uint16 rTokenDist, uint16 rsrDist);

/contracts/interfaces/IRToken.sol Links: 83, 87.

83:	event BasketsNeededChanged(uint192 oldBasketsNeeded, uint192 newBasketsNeeded);
87:	event Melted(uint256 amount);

[NC-09] Error Messages Are Missing From Require Statement(s)

It is good practice to return a message on failure. Error messages help with debugging.

/contracts/plugins/mocks/WETH.sol Links: 43, 68, 71.

43:	require(balanceOf[msg.sender] >= wad);
68:	require(balanceOf[src] >= wad);
71:	require(allowance[src][msg.sender] >= wad);

[NC-10] NatSpec Comments Use // Instead of ///

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

contracts/plugins/mocks/EasyAuction.sol Links: 137, 398.

137:    // @dev: function to intiate a new auction
398:    // @dev function settling the auction and calculating the price

contracts/plugins/mocks/vendor/EasyAuction.sol Links: 337.

337:    // @dev orders are ordered by

[NC-11] Inconsistent Comment Initial Spacing

Some comments have an initial space after // or /// while others do not. It is best for code clearity to keep a consistent style.

The following contracts have both: StaticATokenLM.sol, EasyAuction.sol, and ATokenMock.sol.

All remaining contracts only have initial space comments (EX. // foo).

[NC-12] Power of Ten Literal > 10e3 Not In Scientific Notation

Power of ten literals > 10e3 are easier to read when expressed in scientific notation. Consider expressing large powers of ten in scientific notation (Ex. 10e5).

/contracts/p1/Distributor.sol Links: 165, 166.

165:	require(share.rsrDist <= 10000, "RSR distribution too high");
166:	require(share.rTokenDist <= 10000, "RToken distribution too high");

[NC-13] Non-Assembly Function Available (address().code.length)

The complexity brought by assembly can be avoided by using built in functions when there is no need for assembly. assembly { size := extcodesize(account) } can be replaced by uint256 size = address().code.length.

/contracts/plugins/mocks/vendor/EasyAuction.sol Links: 696.

696:	size := extcodesize(account)

[NC-14] Non-Assembly Function Available (chainid())

The complexity brought by assembly can be avoided by using built in functions when there is no need for assembly. assembly { chainId := chainid() } can be replaced by uint256 chainId = block.chainid.

/contracts/plugins/aave/StaticATokenLM.sol Links: 265.

265:	chainId := chainid()

[NC-15] No License Indication

WETH.sol has a license pasted in full, but is missing an official license indication. If no license is used SPDX-License-Identifier: UNLICENSED should be at the top of a contract.

The following contracts are missing a formal license:

• WETH.sol

[L-01] assert Used Over require

assert should only be used in tests. Consider changing all occurrences of assert to require. Prior to Solidity 0.8 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".

/contracts/p1/BackingManager.sol Links: 249.

249:	assert(tradesOpen == 0 && !basketHandler.fullyCollateralized());

/contracts/p1/BasketHandler.sol Links: 556.

556:	assert(targetIndex < targetsLength);

/contracts/p1/mixins/TradeLib.sol Links: 44, 108, 168, 170.

44:	assert(trade.buyPrice > 0 && trade.buyPrice < FIX_MAX && trade.sellPrice < FIX_MAX);
108:	assert(
168:	assert(errorCode == 0x11 || errorCode == 0x12);
170:	assert(keccak256(reason) == UIntOutofBoundsHash);

/contracts/p1/mixins/RecollateralizationLib.sol Links: 110.

110:	assert(doTrade);

/contracts/p1/mixins/RewardableLib.sol Links: 78, 102.

78:	assert(erc20s[i].balanceOf(address(this)) >= liabilities[erc20s[i]]);
102:	assert(erc20.balanceOf(address(this)) >= liabilities[erc20]);

/contracts/p1/mixins/Trading.sol Links: 114.

114:	assert(address(trades[sell]) == address(0));

/contracts/p1/StRSR.sol Links: 696.

696:	assert(totalStakes + amount < type(uint224).max);

/contracts/plugins/aave/StaticATokenLM.sol Links: 345.

345:	assert(amt == amountToWithdraw);

/contracts/plugins/assets/FiatCollateral.sol Links: 137.

137:	assert(low == 0);

/contracts/plugins/assets/RTokenAsset.sol Links: 74.

74:	assert(low <= high);

/contracts/plugins/assets/Asset.sol Links: 98, 112, 147.

98:	assert(low == 0);
112:	assert(low <= high);
147:	assert(lotLow <= lotHigh);

/contracts/plugins/trading/GnosisTrade.sol Links: 63, 98, 182.

63:	assert(status == TradeStatus.PENDING);
98:	assert(origin_ != address(0));
182:	assert(isAuctionCleared());

[L-02] Spelling Mistake In require Message

On L281 of EasyAuction.sol minimal is misspelled as mimimal. Spelling mistakes in error messages are not good as they can cause confusion in critical circumstances.