QA Report

[code423n4]

Overview Risk Rating	Number of issues
Low Risk	6
Non-Critical Risk	16

Table of Contents

• Low Risk Issues
  • 1. Known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 2. Unsafe casting may overflow
  • 3. Funds could be locked (unused/empty receive() function)
  • 4. Unbounded loop on array can lead to DoS
  • 5. Prevent accidentally burning tokens
  • 6. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
• Non-Critical Issues
  • 1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking
  • 2. It's better to emit after all processing is done
  • 3. Typos in revert strings
  • 4. Typos in comments
  • 5. Deprecated library used for Solidity >= 0.8 : SafeMath
  • 6. Missing friendly revert strings
  • 7. Open TODOS
  • 8. 10000000e18 should be changed to 1e25 for readability reasons
  • 9. 400000e18 should be changed to 4e23 for readability reasons
  • 10. 100000e18 should be changed to 1e23 for readability reasons
  • 11. Use solidity version of at least 0.8.12 to get string.concat()
  • 12. Use bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
  • 13. Use scientific notation (e.g. 1e3, or even just 1000) rather than exponentiation (e.g. 10**3)
  • 14. Adding a return statement when the function defines a named return variable, is redundant
  • 15. The pragmas used are not the same everywhere
  • 16. Non-library/interface files should use fixed compiler versions, not floating ones

Low Risk Issues

1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@3.4.2 here:

• https://github.com/Plex-Engineer/zeroswap/blob/03507a80322112f4f3c723fc68bed0f138702836/package.json#L65

    "@openzeppelin/contracts": "^3.1.0",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

2. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library (or custom safe-casting functions) to prevent unexpected overflows when casting here:

lending-market/contracts/Governance/Comp.sol:64:        balances[account] = uint96(totalSupply);
zeroswap/contracts/uniswapv2/UniswapV2Oracle.sol:59:        return uint8(epochPeriod % granularity);
zeroswap/contracts/uniswapv2/UniswapV2Oracle.sol:120:            uint224((priceCumulativeEnd - priceCumulativeStart) / timeElapsed)

3. Funds could be locked (unused/empty receive() function)

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert. Furthermore, in those contracts, no withdraw mechanism exist to retrieve locked ether (Ether sent by mistake).

lending-market/contracts/Accountant/AccountantDelegate.sol:94:    receive() external override payable {}
lending-market/contracts/Treasury/TreasuryDelegator.sol:130:    receive() external override payable  {}

4. Unbounded loop on array can lead to DoS

As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

lending-market/contracts/Comptroller.sol:160:        accountAssets[borrower].push(cToken);
lending-market/contracts/Comptroller.sol:735:        for (uint i = 0; i < assets.length; i++) {
lending-market/contracts/Comptroller.sol:959:        for (uint i = 0; i < allMarkets.length; i ++) {
lending-market/contracts/Comptroller.sol:962:        allMarkets.push(CToken(cToken));

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

5. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

lending-market/contracts/Treasury/TreasuryDelegate.sol:52:            to.transfer(amount);
lending-market/contracts/Treasury/TreasuryDelegate.sol:56:            note.transfer(recipient, amount);
lending-market/contracts/Comptroller.sol:1380:            comp.transfer(user, amount);

6. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

lending-market/contracts/Governance/Comp.sol:164:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market/contracts/Governance/GovernorAlpha.sol:256:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market/contracts/NoteInterest.sol:95:        uint rand = uint(keccak256(abi.encodePacked(msg.sender))) % 100;
stableswap/contracts/BaseV1-core.sol:424:            abi.encodePacked(
stableswap/contracts/BaseV1-core.sol:525:        bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters
stableswap/contracts/BaseV1-periphery.sol:94:        pair = address(uint160(uint256(keccak256(abi.encodePacked(
stableswap/contracts/BaseV1-periphery.sol:97:            keccak256(abi.encodePacked(token0, token1, stable)),
zeroswap/contracts/uniswapv2/libraries/UniswapV2Library.sol:22:        pair = address(uint(keccak256(abi.encodePacked(
zeroswap/contracts/uniswapv2/libraries/UniswapV2Library.sol:25:                keccak256(abi.encodePacked(token0, token1)),

Non-Critical Issues

1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

lending-market/contracts/Comptroller.sol:214:        assert(assetIndex < len);
lending-market/contracts/Comptroller.sol:360:            assert(markets[cToken].accountMembership[borrower]);
stableswap/contracts/BaseV1-periphery.sol:82:        assert(msg.sender == address(wcanto)); // only accept ETH via fallback from the WETH contract
stableswap/contracts/BaseV1-periphery.sol:227:                assert(amountAOptimal <= amountADesired);
stableswap/contracts/BaseV1-periphery.sol:273:        assert(wcanto.transfer(pair, amountCANTO));
stableswap/contracts/BaseV1-periphery.sol:419:        assert(wcanto.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));

As the Solidity version is > 0.8.* the remaining gas would still be refunded in case of failure.

2. It's better to emit after all processing is done

• lending-market/contracts/CNote.sol:

   18:         emit AccountantSet(accountant_, address(_accountant));
   19       _accountant = AccountantInterface(accountant_);
   20          admin = accountant_;

• lending-market/contracts/Comptroller.sol:

  1124:                 emit CompReceivableUpdated(user, oldReceivable, newReceivable);
  1125  
  1126                  amountToSubtract = currentAccrual;

• lending-market/contracts/NoteInterest.sol:

   73      constructor(uint baseRatePerYear) {
   74          baseRatePerBlock = baseRatePerYear.div(blocksPerYear);
   75:         emit NewInterestParams(baseRatePerBlock);
   76          admin = msg.sender;
   77      }

• lending-market/contracts/Accountant/AccountantDelegate.sol:

  35:   emit AcctInit(cnoteAddress_);
  36  
  37    cnote._setAccountantContract(payable(this));

• lending-market/contracts/Accountant/AccountantDelegator.sol:

  45:         emit NewImplementation(implementation, implementation_);
  46         
  47          implementation = implementation_;

• lending-market/contracts/Governance/Comp.sol:

  228:         emit DelegateChanged(delegator, currentDelegate, delegatee);
  229  
  230          _moveDelegates(currentDelegate, delegatee, delegatorBalance);

  239:         emit Transfer(src, dst, amount);
  240  
  241          _moveDelegates(delegates[src], delegates[dst], amount);

3. Typos in revert strings

• initiatlize

lending-market/contracts/Accountant/AccountantDelegate.sol:29:  require(note.balanceOf(msg.sender) == note._initialSupply(), "AccountantDelegate::initiatlize: Accountant has not received payment");

• arity

lending-market/contracts/Governance/GovernorBravoDelegate.sol:45:                "GovernorBravo::propose: proposal function information arity mismatch");

• faialure

stableswap/contracts/BaseV1-periphery.sol:463:        require(token.code.length > 0, "token code length faialure");

4. Typos in comments

• contructor

lending-market/contracts/Accountant/AccountantDelegate.sol:10:      * @notice Method used to initialize the contract during delegator contructor

• undelrying

lending-market/contracts/CNote.sol:93:     * @param repayAmount the amount of undelrying tokens being returned

• irrelevent

lending-market/contracts/NoteInterest.sol:106:     * @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
lending-market/contracts/NoteInterest.sol:89:     * @notice The following parameters are irrelevent for calculating Note's interest rate. They are passed in to align with the standard function definition `getBorrowRate` ```
- obervations
```solidity
stableswap/contracts/BaseV1-core.sol:62:    // Structure to capture time period obervations every 30 minutes, used for local oracles

5. Deprecated library used for Solidity >= 0.8 : SafeMath

lending-market/contracts/NoteInterest.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/NoteInterest.sol:5:import "./SafeMath.sol";
lending-market/contracts/NoteInterest.sol:15:    using SafeMath for uint;

6. Missing friendly revert strings

lending-market/contracts/Governance/Comp.sol:276:        require(n < 2**32, errorMessage);
lending-market/contracts/Governance/Comp.sol:281:        require(n < 2**96, errorMessage);
lending-market/contracts/Governance/Comp.sol:287:        require(c >= a, errorMessage);
lending-market/contracts/Governance/Comp.sol:292:        require(b <= a, errorMessage);
lending-market/contracts/Treasury/TreasuryDelegate.sol:16:        require(msg.sender == admin);
lending-market/contracts/Treasury/TreasuryDelegate.sol:17:     require(note_ != address(0));
lending-market/contracts/WETH.sol:69:        require(_balanceOf[src] >= wad);
lending-market/contracts/WETH.sol:72:            require(_allowance[src][msg.sender] >= wad);
manifest/contracts/Proposal-Store.sol:31: require(msg.sender == UniGovModAcct);
stableswap/contracts/BaseV1-core.sol:285:        require(!BaseV1Factory(factory).isPaused());
stableswap/contracts/BaseV1-core.sol:465:        require(token.code.length > 0);
stableswap/contracts/BaseV1-core.sol:468:        require(success && (data.length == 0 || abi.decode(data, (bool))));
stableswap/contracts/BaseV1-core.sol:498:        require(msg.sender == pauser);
stableswap/contracts/BaseV1-core.sol:503:        require(msg.sender == pendingPauser);
stableswap/contracts/BaseV1-core.sol:508:        require(msg.sender == pauser);
stableswap/contracts/BaseV1-periphery.sol:210:        require(amountADesired >= amountAMin);
stableswap/contracts/BaseV1-periphery.sol:211:        require(amountBDesired >= amountBMin);
stableswap/contracts/BaseV1-periphery.sol:291:        require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity)); // send liquidity to pair
stableswap/contracts/BaseV1-periphery.sol:456:        require(token.code.length > 0);
stableswap/contracts/BaseV1-periphery.sol:459:        require(success && (data.length == 0 || abi.decode(data, (bool))));

7. Open TODOS

Consider resolving the TODOs before deploying.

lending-market/contracts/Comptroller.sol:1232:        // TODO: Don't distribute supplier COMP if the user is not in the supplier market.
lending-market/contracts/Comptroller.sol:1271:        // TODO: Don't distribute supplier COMP if the user is not in the borrower market.

8. 10000000e18 should be changed to 1e25 for readability reasons

1e25is already well understandable. Writing 10000000e18 is actually confusing

lending-market/contracts/Governance/Comp.sol:15:    uint public constant totalSupply = 10000000e18; // 10 million Comp

9. 400000e18 should be changed to 4e23 for readability reasons

1e23is already well understandable. Writing 400000e18 is actually confusing

lending-market/contracts/Governance/GovernorAlpha.sol:9:    function quorumVotes() public pure returns (uint) { return 400000e18; } // 400,000 = 4% of Comp

10. 100000e18 should be changed to 1e23 for readability reasons

1e23is already well understandable. Writing 100000e18 is actually confusing

lending-market/contracts/Governance/GovernorAlpha.sol:12:    function proposalThreshold() public pure returns (uint) { return 100000e18; } // 100,000 = 1% of Comp

11. Use solidity version of at least 0.8.12 to get string.concat()

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

lending-market/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/Comp.sol:164:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorAlpha.sol:256:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
stableswap/contracts/BaseV1-core.sol:2:pragma solidity 0.8.11;
stableswap/contracts/BaseV1-core.sol:109:            name = string(abi.encodePacked("StableV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
stableswap/contracts/BaseV1-core.sol:110:            symbol = string(abi.encodePacked("sAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
stableswap/contracts/BaseV1-core.sol:112:            name = string(abi.encodePacked("VolatileV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
stableswap/contracts/BaseV1-core.sol:113:            symbol = string(abi.encodePacked("vAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
stableswap/contracts/BaseV1-core.sol:424:            abi.encodePacked(
stableswap/contracts/BaseV1-core.sol:525:        bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters
stableswap/contracts/BaseV1-periphery.sol:3:pragma solidity 0.8.11;
stableswap/contracts/BaseV1-periphery.sol:94:        pair = address(uint160(uint256(keccak256(abi.encodePacked(
stableswap/contracts/BaseV1-periphery.sol:97:            keccak256(abi.encodePacked(token0, token1, stable)),

12. Use bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

stableswap/contracts/BaseV1-core.sol:2:pragma solidity 0.8.11;
stableswap/contracts/BaseV1-core.sol:525:        bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters
stableswap/contracts/BaseV1-periphery.sol:3:pragma solidity 0.8.11;
stableswap/contracts/BaseV1-periphery.sol:97:            keccak256(abi.encodePacked(token0, token1, stable)),

13. Use scientific notation (e.g. 1e3, or even just 1000) rather than exponentiation (e.g. 10**3)

stableswap/contracts/BaseV1-core.sol:56:    uint internal constant MINIMUM_LIQUIDITY = 10**3;
stableswap/contracts/BaseV1-periphery.sol:67:    uint internal constant MINIMUM_LIQUIDITY = 10**3;

14. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

• lending-market/contracts/Governance/GovernorAlpha.sol:

  218:     function getActions(uint proposalId) public view returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas) {
  220:         return (p.targets, p.values, p.signatures, p.calldatas);

• lending-market/contracts/Governance/GovernorBravoDelegate.sol:

  104:     function getActions(uint proposalId) external view returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas) {
  106:         return (p.targets, p.values, p.signatures, p.calldatas);

• stableswap/contracts/BaseV1-periphery.sol:

  117:     function getAmountOut(uint amountIn, address tokenIn, address tokenOut) external view returns (uint amount, bool stable) {
  128:         return amountStable > amountVolatile ? (amountStable, true) : (amountVolatile, false);

• zeroswap/contracts/uniswapv2/UniswapV2Oracle.sol:

   57:     function observationIndexOf(uint timestamp) public view returns (uint8 index) {
   59:         return uint8(epochPeriod % granularity);

  128:     function consult(address tokenIn, uint amountIn, address tokenOut) external view returns (uint amountOut) {
  141:             return computeAmountOut(firstObservation.price0Cumulative, price0Cumulative, timeElapsed, amountIn);
  143:             return computeAmountOut(firstObservation.price1Cumulative, price1Cumulative, timeElapsed, amountIn);

15. The pragmas used are not the same everywhere

lending-market/contracts/Accountant/AccountantDelegate.sol:1:pragma solidity ^0.8.10;
manifest/contracts/Proposal-Store.sol:3:pragma solidity ^0.8.0;
stableswap/contracts/BaseV1-core.sol:2:pragma solidity 0.8.11;
zeroswap/contracts/uniswapv2/libraries/UniswapV2Library.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/libraries/UniswapV2OracleLibrary.sol:3:pragma solidity =0.6.12;

16. Non-library/interface files should use fixed compiler versions, not floating ones

lending-market/contracts/Accountant/AccountantDelegate.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Accountant/AccountantDelegator.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Accountant/AccountantInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoDelegate.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoDelegator.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryDelegate.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryDelegator.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/CNote.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Comptroller.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/NoteInterest.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/WETH.sol:1:pragma solidity ^0.8.10;
manifest/contracts/Proposal-Store.sol:3:pragma solidity ^0.8.0;

[GalloDaSballo]

1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

Not a fan of the blanket statement, in lack of any risk am downgrading to NC

2. Unsafe casting may overflow

Valid Low, esp on uint96

3. Funds could be locked (unused/empty receive() function)

Valid Low

4. Unbounded loop on array can lead to DoS

I think this is highly unlikely, am very conflicted. Marking Low for now

5. Prevent accidentally burning tokens

Valid Low

6. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

I don't see any risk of clash of hashes, so in lack of further detail am marking invalid, consider finding an instance of clash and submit as higher severity in the future

1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Valid Ref

2. It's better to emit after all processing is done

Disagree as long as usage is consistent (+ slither gives false positives is you emit after a transfer or w/e)

3. Typos in revert strings and 4.

NC

5. Deprecated library used for Solidity >= 0.8 : SafeMath

Valid R

6. Missing friendly revert strings

Valid NC

7. Open TODOS

Valid NC

8. 10000000e18 should be changed to 1e25 for readability reasons and others

Valid Ref

11. Use solidity version of at least 0.8.12 to get string.concat() and bytes.concat

NC

14. Adding a return statement when the function defines a named return variable, is redundant

Valid Ref

15. The pragmas used are not the same everywhere & Interfaces

Valid NC

Neat report

4L 4R 6NC