QA Report #150

code423n4 · 2022-07-02T13:12:16Z

Require Statements without Descriptive Revert Strings
Unnecessary Use of Named Returns
Should Resolve TODOs before Deployment
Constant Naming Convention
Define Magic Numbers to Constant
Use Double-Quotes for Strings
Best Practices of Source File Layout
Event is Missing Indexed Fields
TYPO
Incomplete NatSpec
Use fixed compiler versions instead of floating version

Low Risk Issues

[L-01] Admin Address Change should be a 2-Step Process

Issue

The function changes an admin account with single process.
This can be a concern since an admin role has a high privilege in the contract and
mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Note.sol#L21-L27
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/NoteInterest.sol#L109-L114

Mitigation

This can be fixed by implementing 2-step process. We can do this by following.
First make the function approve a new address as a pending admin.
Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

[L-02] Require should be used instead of Assert

Issue

Solidity documents mention that properly functioning code should never reach a failing assert statement
and if this happens there is a bug in the contract which should be fixed.
Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#panic-via-assert-and-error-via-require

PoC

./BaseV1-periphery.sol:100:        assert(msg.sender == address(wcanto)); // only accept ETH via fallback from the WETH contract
./BaseV1-periphery.sol:245:                assert(amountAOptimal <= amountADesired);
./BaseV1-periphery.sol:291:        assert(wcanto.transfer(pair, amountCANTO));
./BaseV1-periphery.sol:437:        assert(wcanto.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));

Mitigation

Replace assert by require.

[L-03] Immutable addresses should 0-Check

Issue

I recommend adding check of 0-address for immutable addresses.
Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

PoC

4 immutable addresses of "USDC", "Comptroller", "factory" and "wcanto" of BaseV1-periphery.sol
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L91-L97

91:    constructor(address _factory, address _wcanto, address USDC_, address Comptroller_) {
92:        factory = _factory;
93:        pairCodeHash = IBaseV1Factory(_factory).pairCodeHash();
94:        wcanto = IWCANTO(_wcanto);
95:        USDC = USDC_;
96:        Comptroller = Comptroller_;
97:    }

Mitigation

Add 0-address check for above 4 immutable addresses.

Non-critical Issues

[N-01] Require Statements without Descriptive Revert Strings

Issue

It is best practice to include descriptive revert strings for require statement for readability and auditing.

PoC

./Note.sol:22:        require(msg.sender == admin);
./Note.sol:23:        require(address(accountant) == address(0));
./AccountantDelegator.sol:22:        require(admin_ != address(0));
./CNote.sol:74:        require(address(_accountant) != address(0)); //check that the accountant has been set
./CNote.sol:121:        require(address(_accountant) != address(0)); //check that the accountant has been set
./TreasuryDelegator.sol:13:        require(admin_ != address(0));
./Reservoir.sol:74:    require(c >= a, errorMessage);
./Reservoir.sol:79:    require(b <= a, errorMessage);
./Reservoir.sol:90:    require(c / a == b, errorMessage);
./GovernorBravoDelegate.sol:53:        require(proposals[unigovProposal.id].id == 0);
./BaseV1-core.sol:127:        require(_unlocked == 1);
./BaseV1-core.sol:288:        require(!BaseV1Factory(factory).isPaused());
./BaseV1-core.sol:468:        require(token.code.length > 0);
./BaseV1-core.sol:471:        require(success && (data.length == 0 || abi.decode(data, (bool))));
./BaseV1-core.sol:501:        require(msg.sender == pauser);
./BaseV1-core.sol:506:        require(msg.sender == pendingPauser);
./BaseV1-core.sol:511:        require(msg.sender == pauser);
./Comp.sol:276:        require(n < 2**32, errorMessage);
./Comp.sol:281:        require(n < 2**96, errorMessage);
./Comp.sol:287:        require(c >= a, errorMessage);
./Comp.sol:292:        require(b <= a, errorMessage);
./BaseV1-periphery.sol:228:        require(amountADesired >= amountAMin);
./BaseV1-periphery.sol:229:        require(amountBDesired >= amountBMin);
./BaseV1-periphery.sol:309:        require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity)); // send liquidity to pair
./BaseV1-periphery.sol:474:        require(token.code.length > 0);
./BaseV1-periphery.sol:477:        require(success && (data.length == 0 || abi.decode(data, (bool))));

Mitigation

Add descriptive revert strings to easier understand what the code is trying to do.

[N-02] Unnecessary Use of Named Returns

Issue

Several function adds return statement even thought named returns variable are used.
Remove unnecessary named returns variable to improve code readability.
Also keeping the use of named returns or return statement consistent through out the whole project
if possible is recommended.

PoC

Mitigation

Remove unused named returns variable and keep the use of named returns or return statement consistent
through out the whole project if possible.

[N-03] Should Resolve TODOs before Deployment

Issue

Questions/Issues in the code should be resolved before the deployment.

PoC

./Reservoir.sol:49:    uint reservoirBalance_ = token_.balanceOf(address(this)); // TODO: Verify this is a static call

Mitigation

Resolve the question/issue and remove TODO comment from code.

[N-04] Constant Naming Convention

Issue

Constants should be named with all capital letters with underscores separating words.
Reference: https://docs.soliditylang.org/en/v0.8.15/style-guide.html?highlight=naming#constants

PoC

./GovernorAlpha.sol:6:    string public constant name = "Compound Governor Alpha";
./NoteInterest.sol:20:    uint public constant BlocksPerYear = 5256000;
./GovernorBravoDelegate.sol:9:    string public constant name = "Compound Governor Bravo";
./GovernorBravoDelegate.sol:12:    uint public constant proposalMaxOperations = 10; // 10 actions
./BaseV1-core.sol:43:    uint8 public constant decimals = 18;
./BaseV1-core.sol:72:    uint constant periodSize = 0;
./Comp.sol:6:    string public constant name = "Compound";
./Comp.sol:9:    string public constant symbol = "COMP";
./Comp.sol:12:    uint8 public constant decimals = 18;
./Comp.sol:15:    uint public constant totalSupply = 10000000e18; // 10 million Comp
./TreasuryDelegate.sol:12:    bytes32 constant cantoDenom = keccak256(bytes("CANTO"));
./TreasuryDelegate.sol:13:    bytes32 constant noteDenom = keccak256(bytes("NOTE")); //cache hashed values to reduce unnecessary gas costs

Mitigation

Name the constants with all capital letters with underscores separating words.
For examples: NAME, BLOCKS_PER_YEAR, DECIMALS

[N-05] Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number.
This improves code readability and maintainability.

PoC

./BaseV1-core.sol:332:        return x0*(y*y/1e18*y/1e18)/1e18+(x0*x0/1e18*x0/1e18)*y/1e18;

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L332

./BaseV1-core.sol:336:        return 3*x0*(y*y/1e18)/1e18+(x0*x0/1e18*x0/1e18);

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L336

./BaseV1-periphery.sol:534:        return price * 1e18;

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L534

Mitigation

Define magic numbers to constant.

[N-06] Use Double-Quotes for Strings

Issue

Strings should be quoted with double-quotes instead of single-quotes.
Solidity documents reference: https://docs.soliditylang.org/en/v0.8.15/style-guide.html?highlight=strings#other-recommendations

PoC

./BaseV1-core.sol:256:        require(liquidity > 0, 'ILM'); // BaseV1: INSUFFICIENT_LIQUIDITY_MINTED
./BaseV1-core.sol:275:        require(amount0 > 0 && amount1 > 0, 'ILB'); // BaseV1: INSUFFICIENT_LIQUIDITY_BURNED
./BaseV1-core.sol:289:        require(amount0Out > 0 || amount1Out > 0, 'IOA'); // BaseV1: INSUFFICIENT_OUTPUT_AMOUNT
./BaseV1-core.sol:291:        require(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL'); // BaseV1: INSUFFICIENT_LIQUIDITY
./BaseV1-core.sol:297:        require(to != _token0 && to != _token1, 'IT'); // BaseV1: INVALID_TO
./BaseV1-core.sol:306:        require(amount0In > 0 || amount1In > 0, 'IIA'); // BaseV1: INSUFFICIENT_INPUT_AMOUNT
./BaseV1-core.sol:312:        require(_k(_balance0, _balance1) >= _k(_reserve0, _reserve1), 'K'); // BaseV1: K
./BaseV1-core.sol:416:        require(deadline >= block.timestamp, 'BaseV1: EXPIRED');
./BaseV1-core.sol:434:        require(recoveredAddress != address(0) && recoveredAddress == owner, 'BaseV1: INVALID_SIGNATURE');
./BaseV1-core.sol:524:        require(tokenA != tokenB, 'IA'); // BaseV1: IDENTICAL_ADDRESSES
./BaseV1-core.sol:526:        require(token0 != address(0), 'ZA'); // BaseV1: ZERO_ADDRESS
./BaseV1-core.sol:527:        require(getPair[token0][token1][stable] == address(0), 'PE'); // BaseV1: PAIR_EXISTS - single check is sufficient

Mitigation

Change above single-quotes to double-quotes.

[N-07] Best Practices of Source File Layout

Issue

I recommend following best practices of solidity source file layout for readability.
Reference: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#order-of-layout

This best practices is to layout a contract elements in following order:
Pragma statements, Import statements, Interfaces, Libraries, Contracts

Inside each contract, library or interface, use the following order:
Type declarations, State variables, Events, Modifiers, Functions

PoC

CNote.sol

Modifier placed at the end of contract

CToken.sol

Modifier placed at the end of contract

GovernorAlpha.sol

Interfaces placed after Contracts

GovernorBravoInterfaces.sol

Interfaces placed after Contracts

Reservoir.sol

Import statement placed very end of the layout

Mitigation

I recommend to follow best practice source file layout

[N-08] Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./AccountantInterfaces.sol:24:    event AcctInit(address lendingMarketAddress);
./AccountantInterfaces.sol:25:	event AcctSupplied(uint amount, uint err);
./AccountantInterfaces.sol:26:    event AcctRedeemed(uint amount);
./AccountantInterfaces.sol:36:    event NewImplementation(address oldImplementation, address newImplementation);
./GovernorAlpha.sol:116:    event ProposalCreated(uint id, address proposer, address[] targets, uint[] values, string[] signatures, bytes[] calldatas, uint startBlock, uint endBlock, string description);
./GovernorAlpha.sol:119:    event VoteCast(address voter, uint proposalId, bool support, uint votes);
./GovernorAlpha.sol:122:    event ProposalCanceled(uint id);
./GovernorAlpha.sol:125:    event ProposalQueued(uint id, uint eta);
./GovernorAlpha.sol:128:    event ProposalExecuted(uint id);
./NoteInterest.sol:64:    event NewBaseRate(uint oldBaseRateMantissa, uint newBaseRateMantissa);
./NoteInterest.sol:67:    event NewAdjusterCoefficient(uint oldAdjusterCoefficient, uint newAdjusterCoefficient);
./NoteInterest.sol:70:    event NewUpdateFrequency(uint oldUpdateFrequency, uint newUpdateFrequency);
./NoteInterest.sol:73:    event NewInterestParams(uint baserateperblock);
./NoteInterest.sol:76:    event NewPriceOracle(address oldOracle, address newOracle);
./NoteInterest.sol:79:    event NewAdmin(address oldAdmin, address newAdmin);
./CNote.sol:10:    event AccountantSet(address accountant, address accountantPrior);
./GovernorBravoInterfaces.sol:9:    event ProposalCreated(uint id, address proposer, address[] targets, uint[] values, string[] signatures, bytes[] calldatas, uint startBlock, uint endBlock, string description);
./GovernorBravoInterfaces.sol:12:    event ProposalCanceled(uint id);
./GovernorBravoInterfaces.sol:15:    event ProposalQueued(uint id, uint eta);
./GovernorBravoInterfaces.sol:18:    event ProposalExecuted(uint id);
./GovernorBravoInterfaces.sol:21:    event NewImplementation(address oldImplementation, address newImplementation);
./GovernorBravoInterfaces.sol:24:    event ProposalThresholdSet(uint oldProposalThreshold, uint newProposalThreshold);
./GovernorBravoInterfaces.sol:27:    event NewPendingAdmin(address oldPendingAdmin, address newPendingAdmin);
./GovernorBravoInterfaces.sol:30:    event NewAdmin(address oldAdmin, address newAdmin);
./BaseV1-core.sol:90:    event Mint(address indexed sender, uint amount0, uint amount1);
./BaseV1-core.sol:91:    event Burn(address indexed sender, uint amount0, uint amount1, address indexed to);
./BaseV1-core.sol:92-99:    event Swap(address indexed sender, uint amount0In, uint amount1In, uint amount0Out, uint amount1Out, address indexed to);
./BaseV1-core.sol:100:    event Sync(uint reserve0, uint reserve1);
./BaseV1-core.sol:101:    event Claim(address indexed sender, address indexed recipient, uint amount0, uint amount1);
./BaseV1-core.sol:103:    event Transfer(address indexed from, address indexed to, uint amount);
./BaseV1-core.sol:104:    event Approval(address indexed owner, address indexed spender, uint amount);
./BaseV1-core.sol:489:    event PairCreated(address indexed token0, address indexed token1, bool stable, address pair, uint);
./Comp.sol:51:    event DelegateVotesChanged(address indexed delegate, uint previousBalance, uint newBalance);
./Comp.sol:54:    event Transfer(address indexed from, address indexed to, uint256 amount);
./Comp.sol:57:    event Approval(address indexed owner, address indexed spender, uint256 amount);
./TreasuryInterfaces.sol:22:    event NewImplementation(address oldImplementation, address newImplementation);
./TreasuryInterfaces.sol:23:    event Received(address sender, uint amount);

Mitigation

Use all 3 index fields when possible.

[N-09] TYPO

Issue

Some typo was found in the following.

PoC

BaseV1-core.sol

Change "obervations" to "observations"
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L64

64:     // Structure to capture time period obervations every 30 minutes, used for local oracles

BaseV1-periphery.sol

Change "doesn"t" to "doesn't"
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L173
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L204
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L230

173:        // create the pair if it doesn"t exist yet
204:        // create the pair if it doesn"t exist yet
230:        // create the pair if it doesn"t exist yet

CNote.sol

Change "Accounant" to "Accountant"
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CNote.sol#L24

24:    * @dev return the current address of the Accounant

Change "efore" to "before"
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CNote.sol#L41

41:     * @dev This function does not accrue efore calculating the exchange rate

NoteInterest.sol

Change "irrelevent" to "irrelevant"
https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/NoteInterest.sol#L124

124:     * @notice The following parameters are irrelevent for calculating Note's interest rate. They are passed in to align with the standard function definition `getSupplyRate` in InterestRateModel

Mitigation

Change to correct spelling as mentioned in above PoC

[N-10] Incomplete NatSpec

Issue

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces.
However there were places where this NatSpec was incomplete.

PoC

Mitigation

Add missing NatSpec mentioned in above PoC.

[N-11] Use fixed compiler versions instead of floating version

Issue

It is best practice to lock your pragma instead of using floating pragma.
The use of floating pragma has a risk of accidentally get deployed using latest complier
which may have higher risk of undiscovered bugs.
Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

./Note.sol:1:pragma solidity ^0.8.10;
./AccountantDelegator.sol:1:pragma solidity ^0.8.10;
./AccountantInterfaces.sol:1:pragma solidity ^0.8.10;
./CToken.sol:2:pragma solidity ^0.8.10;
./GovernorAlpha.sol:2:pragma solidity ^0.8.10;
./NoteInterest.sol:1:pragma solidity ^0.8.10;
./CNote.sol:1:pragma solidity ^0.8.10;
./TreasuryDelegator.sol:1:pragma solidity ^0.8.10;
./Reservoir.sol:2:pragma solidity ^0.8.10;
./GovernorBravoInterfaces.sol:2:pragma solidity ^0.8.10;
./GovernorBravoDelegate.sol:2:pragma solidity ^0.8.10;
./Comp.sol:2:pragma solidity ^0.8.10;
./GovernorBravoDelegator.sol:2:pragma solidity ^0.8.10;
./TreasuryInterfaces.sol:1:pragma solidity ^0.8.10;
./AccountantDelegate.sol:1:pragma solidity ^0.8.10;
./TreasuryDelegate.sol:1:pragma solidity ^0.8.10;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

The text was updated successfully, but these errors were encountered:

GalloDaSballo · 2022-08-14T19:55:09Z

Admin Address Change should be a 2-Step Process

NC

Require should be used instead of Assert

Valid Low

Immutable addresses should 0-Check

Valid Low

Require Statements without Descriptive Revert Strings

NC

Unnecessary Use of Named Returns

Valid Refactoring

Should Resolve TODOs before Deployment

NC

Constant Naming Convention

Valid R

Define Magic Numbers to Constant

Given the cases, NC

Use Double-Quotes for Strings

Valid R

Best Practices of Source File Layout

Valid R

Event is Missing Indexed Fields

Disagree with the events shown needing indexing

TYPO

NC

Incomplete NatSpec

NC

Use fixed compiler versions instead of floating version

NC

GalloDaSballo · 2022-08-14T19:55:51Z

2L 4R 7 NC

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 2, 2022

code423n4 added a commit that referenced this issue Jul 2, 2022

TomJ issue #150

1feb517

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #150

QA Report #150

code423n4 commented Jul 2, 2022

GalloDaSballo commented Aug 14, 2022

GalloDaSballo commented Aug 14, 2022

QA Report #150

QA Report #150

Comments

code423n4 commented Jul 2, 2022

Table of Contents

Low Risk Issues

Non-critical Issues

Low Risk Issues

[L-01] Admin Address Change should be a 2-Step Process

Issue

PoC

Mitigation

[L-02] Require should be used instead of Assert

Issue

PoC

Mitigation

[L-03] Immutable addresses should 0-Check

Issue

PoC

Mitigation

Non-critical Issues

[N-01] Require Statements without Descriptive Revert Strings

Issue

PoC

Mitigation

[N-02] Unnecessary Use of Named Returns

Issue

PoC

Mitigation

[N-03] Should Resolve TODOs before Deployment

Issue

PoC

Mitigation

[N-04] Constant Naming Convention

Issue

PoC

Mitigation

[N-05] Define Magic Numbers to Constant

Issue

PoC

Mitigation

[N-06] Use Double-Quotes for Strings

Issue

PoC

Mitigation

[N-07] Best Practices of Source File Layout

Issue

PoC

Mitigation

[N-08] Event is Missing Indexed Fields

Issue

PoC

Mitigation

[N-09] TYPO

Issue

PoC

Mitigation

[N-10] Incomplete NatSpec

Issue

PoC

Mitigation

[N-11] Use fixed compiler versions instead of floating version

Issue

PoC

Mitigation

GalloDaSballo commented Aug 14, 2022

Admin Address Change should be a 2-Step Process

Require should be used instead of Assert

Immutable addresses should 0-Check

Require Statements without Descriptive Revert Strings

Unnecessary Use of Named Returns

Should Resolve TODOs before Deployment

Constant Naming Convention

Define Magic Numbers to Constant

Use Double-Quotes for Strings

Best Practices of Source File Layout

Event is Missing Indexed Fields

TYPO

Incomplete NatSpec

Use fixed compiler versions instead of floating version