search

code-423n4 / 2022-06-canto-v2-findings

0 stars 0 forks source link

QA Report #59

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
Overview Risk Rating Number of issues
Low Risk 2
Non-Critical Risk 11

Table of Contents

1. Low Risk Issues

1.1. Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack and failing for certain token implementations that do not return a boolean value. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

lending-market-v2/contracts/Accountant/AccountantDelegate.sol:41:  note.approve(cnoteAddress_, type(uint).max); // approve lending market, to transferFrom Accountant as needed
lending-market-v2/contracts/CDaiDelegate.sol:57:        dai.approve(daiJoinAddress, type(uint).max);

1.2. 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-v2/contracts/Governance/Comp.sol:164:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market-v2/contracts/Governance/GovernorAlpha.sol:256:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:427:            abi.encodePacked(
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:528:        bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:112:        pair = address(uint160(uint256(keccak256(abi.encodePacked(
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:115:            keccak256(abi.encodePacked(token0, token1, stable)),
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:539:        return (keccak256(abi.encodePacked(str1)) == keccak256(abi.encodePacked(str2)));
lending-market-v2/contracts/SimplePriceOracle.sol:42:        return (keccak256(abi.encodePacked((a))) == keccak256(abi.encodePacked((b))));
lending-market-v2/contracts/Timelock.sol:96:            callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);

2. Non-Critical Issues

2.1. import "hardhat/console.sol"; shouldn't exist in production code

Please delete the following before deploying:

lending-market-v2/contracts/CErc20.sol:
  5: import "hardhat/console.sol";

lending-market-v2/contracts/Accountant/AccountantDelegate.sol:
  7: import "hardhat/console.sol";

lending-market-v2/contracts/Stableswap/BaseV1-core.sol:
    4: import "hardhat/console.sol";
  207:         console.log("tokenIn: ", tokenIn);

2.2. 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-v2/contracts/Stableswap/BaseV1-periphery.sol:100:        assert(msg.sender == address(wcanto)); // only accept ETH via fallback from the WETH contract
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:245:                assert(amountAOptimal <= amountADesired);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:291:        assert(wcanto.transfer(pair, amountCANTO));
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:437:        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.3. It's better to emit after all processing is done

lending-market-v2/contracts/CNote.sol:
  16      function setAccountantContract(address accountant_) public {
  17          require(msg.sender == admin, "CNote::_setAccountantContract:Only admin may call this function");
  18          
  19:         emit AccountantSet(accountant_, address(_accountant));
  20       _accountant = AccountantInterface(accountant_);
  21      }
lending-market-v2/contracts/NoteInterest.sol:
   91      constructor(uint _baseRatePerYear) {
   92          baseRatePerYear = _baseRatePerYear;
   93          baseRatePerBlock = _baseRatePerYear.div(BlocksPerYear);
   94:         emit NewInterestParams(baseRatePerBlock);
   95          admin = msg.sender;
   96          lastUpdateBlock = block.number;
   97      }

lending-market-v2/contracts/SimplePriceOracle.sol:
  25      function setUnderlyingPrice(CToken cToken, uint underlyingPriceMantissa) public {
  26          address asset = _getUnderlyingAddress(cToken);
  27:         emit PricePosted(asset, prices[asset], underlyingPriceMantissa, underlyingPriceMantissa);
  28          prices[asset] = underlyingPriceMantissa;
  29      }

  31      function setDirectPrice(address asset, uint price) public {
  32:         emit PricePosted(asset, prices[asset], price, price);
  33          prices[asset] = price;
  34      }

lending-market-v2/contracts/Accountant/AccountantDelegate.sol:
  34  
  35    uint[] memory err = comptroller.enterMarkets(MarketEntered); // check if market entry returns without error
  36    if (err[0] != 0) {
  37     revert ErrorMarketEntering(err[0]);
  38    }
  39:   emit AcctInit(cnoteAddress_);
  40  
  41    note.approve(cnoteAddress_, type(uint).max); // approve lending market, to transferFrom Accountant as needed
  42   }

lending-market-v2/contracts/Accountant/AccountantDelegator.sol:
  44:         emit NewImplementation(implementation, implementation_);
  45         
  46          implementation = implementation_;
  47      }

lending-market-v2/contracts/Governance/Comp.sol:
  137:             emit Approval(src, spender, newAllowance);
  138          }
  139  
  140          _transferTokens(src, dst, amount);
  141          return true;
  142      }

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

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

lending-market-v2/contracts/Stableswap/BaseV1-core.sol:
  453:             emit Approval(src, spender, newAllowance);
  454          }
  455  
  456          _transferTokens(src, dst, amount);
  457          return true;
  458      }

2.4. Typos

lending-market-v2/contracts/Accountant/AccountantDelegate.sol:12:      * @notice Method used to initialize the contract during delegator contructor
lending-market-v2/contracts/Governance/GovernorAlpha.sol:138:        require(targets.length == values.length && targets.length == signatures.length && targets.length == calldatas.length, "GovernorAlpha::propose: proposal function information arity mismatch");
lending-market-v2/contracts/Governance/GovernorAlpha.sol:156:        require(newProposal.id == 0, "GovernorAlpha::propose: ProposalID collsion");
lending-market-v2/contracts/Governance/GovernorBravoDelegate.sol:45:                "GovernorBravo::propose: proposal function information arity mismatch");
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:64:    // Structure to capture time period obervations every 30 minutes, used for local oracles
lending-market-v2/contracts/CNote.sol:24:    * @dev return the current address of the Accounant
lending-market-v2/contracts/CToken.sol:291:     * @dev This function does not accrue efore calculating the exchange rate
lending-market-v2/contracts/DAIInterestRateModelV3.sol:65:     * @param reserves The total amnount of reserves the market has

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

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

2.6. Missing friendly revert strings

lending-market-v2/contracts/Accountant/AccountantDelegator.sol:22:        require(admin_ != address(0));
lending-market-v2/contracts/Governance/Comp.sol:276:        require(n < 2**32, errorMessage);
lending-market-v2/contracts/Governance/Comp.sol:281:        require(n < 2**96, errorMessage);
lending-market-v2/contracts/Governance/Comp.sol:287:        require(c >= a, errorMessage);
lending-market-v2/contracts/Governance/Comp.sol:292:        require(b <= a, errorMessage);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:127:        require(_unlocked == 1);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:288:        require(!BaseV1Factory(factory).isPaused());
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:468:        require(token.code.length > 0);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:471:        require(success && (data.length == 0 || abi.decode(data, (bool))));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:501:        require(msg.sender == pauser);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:506:        require(msg.sender == pendingPauser);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:511:        require(msg.sender == pauser);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:228:        require(amountADesired >= amountAMin);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:229:        require(amountBDesired >= amountBMin);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:309:        require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity)); // send liquidity to pair
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:474:        require(token.code.length > 0);
lending-market-v2/contracts/Treasury/TreasuryDelegator.sol:13:        require(admin_ != address(0));
lending-market-v2/contracts/Note.sol:22:        require(msg.sender == admin);
lending-market-v2/contracts/Note.sol:23:        require(address(accountant) == address(0));
manifest-v2/contracts/Proposal-Store.sol:44:        require(msg.sender == UniGovModAcct);

2.7. Open TODOS

Consider resolving the TODOs before deploying.

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

2.8. 1000000000000000000 should be changed to 1e18 for readability reasons

manifest-v2/contracts/ERC20MaliciousDelayed.sol:11:  uint256 private _bigNum = 1000000000000000000; // ~uint256(0)

2.9. Use string.concat() or bytes.concat()

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

Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

lending-market-v2/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/Comp.sol:164:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market-v2/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorAlpha.sol:256:        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:2:pragma solidity 0.8.11;
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:111:            name = string(abi.encodePacked("StableV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:112:            symbol = string(abi.encodePacked("sAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:114:            name = string(abi.encodePacked("VolatileV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:115:            symbol = string(abi.encodePacked("vAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:528:        bytes32 salt = keccak256(abi.encodePacked(token0, token1, stable)); // notice salt includes stable as well, 3 parameters
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:3:pragma solidity 0.8.11;
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:112:        pair = address(uint160(uint256(keccak256(abi.encodePacked(
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:115:            keccak256(abi.encodePacked(token0, token1, stable)),
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:539:        return (keccak256(abi.encodePacked(str1)) == keccak256(abi.encodePacked(str2)));
lending-market-v2/contracts/SimplePriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/SimplePriceOracle.sol:42:        return (keccak256(abi.encodePacked((a))) == keccak256(abi.encodePacked((b))));
lending-market-v2/contracts/Timelock.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Timelock.sol:96:            callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);

2.10. The pragmas used are not the same everywhere

lending-market-v2/contracts/Stableswap/BaseV1-core.sol:2:pragma solidity 0.8.11;
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:3:pragma solidity 0.8.11;
lending-market-v2/contracts/Accountant/AccountantDelegate.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Accountant/AccountantDelegator.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Accountant/AccountantInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoDelegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoDelegator.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryDelegate.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryDelegator.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/BaseJumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CDaiDelegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Delegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Delegator.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Immutable.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CEther.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CNote.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/CToken.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CTokenInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/DAIInterestRateModelV3.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/EIP20Interface.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/EIP20NonStandardInterface.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/ERC20.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/ErrorReporter.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/ExponentialNoError.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/InterestRateModel.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/JumpRateModel.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/JumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Maximillion.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Note.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/NoteInterest.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/PriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Reservoir.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/SafeMath.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/SimplePriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Timelock.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/WETH.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/WhitePaperInterestRateModel.sol:2:pragma solidity ^0.8.10;
manifest-v2/contracts/ERC20Burnable.sol:4:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20DirectBalanceManipulation.sol:3:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20MaliciousDelayed.sol:3:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20MinterBurnerDecimals.sol:4:pragma solidity ^0.8.0;
manifest-v2/contracts/Proposal-Store.sol:3:pragma solidity ^0.8.0;

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

lending-market-v2/contracts/Accountant/AccountantDelegate.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Accountant/AccountantDelegator.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Accountant/AccountantInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoDelegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoDelegator.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Governance/GovernorBravoInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryDelegate.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryDelegator.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/Treasury/TreasuryInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/BaseJumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CDaiDelegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Delegate.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Delegator.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CErc20Immutable.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CEther.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CNote.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/CToken.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/CTokenInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/DAIInterestRateModelV3.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/EIP20Interface.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/EIP20NonStandardInterface.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/ERC20.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/ErrorReporter.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/ExponentialNoError.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/InterestRateModel.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/JumpRateModel.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/JumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Maximillion.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Note.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/NoteInterest.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/PriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Reservoir.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/SafeMath.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/SimplePriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/Timelock.sol:2:pragma solidity ^0.8.10;
lending-market-v2/contracts/WETH.sol:1:pragma solidity ^0.8.10;
lending-market-v2/contracts/WhitePaperInterestRateModel.sol:2:pragma solidity ^0.8.10;
manifest-v2/contracts/ERC20Burnable.sol:4:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20DirectBalanceManipulation.sol:3:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20MaliciousDelayed.sol:3:pragma solidity ^0.8.0;
manifest-v2/contracts/ERC20MinterBurnerDecimals.sol:4:pragma solidity ^0.8.0;
manifest-v2/contracts/Proposal-Store.sol:3:pragma solidity ^0.8.0;
GalloDaSballo commented 1 year ago

1.1. Deprecated approve() function

Disagree for this case (0 -> max)

1.2. abi.encodePacked()

Disputed in lack of POC, all the listed lines will not allow "dynamic types" and sorting of pairs is determinsitic

2.1. import "hardhat/console.sol"; shouldn't exist in production code

Valid R

2.2. require() should be used for checking error

Valid Low

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

Disagree as Slither will give false positive, is preference, ultimately up to developer

2.4. Typos

NC

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

Valid R

2.6. Missing friendly revert strings

NC

2.8. 1000000000000000000 should be changed to 1e18 for readability reasons

R

2.9. Use string.concat() or bytes.concat()

NC

2.10. The pragmas used are not the same everywhere & 2.11

NC

GalloDaSballo commented 1 year ago

1L 3R 4NC