search

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

0 stars 0 forks source link

QA Report #170

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

The general concerns are with the use of deprecated methods:

assert statement should not be used

IMPACT

Properly functioning code should never reach a failing assert statement. If it happened, it would indicate the presence of a bug in the contract. A failing assert uses all the remaining gas, which can be financially painful for a user.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

lending-market/Comptroller.sol

l214 assert(assetIndex < len)
l360 assert(markets[cToken].accountMembership[borrower])

stableswap/BaseV1-periphery.sol

l82 assert(msg.sender == address(wcanto))
l227 assert(amountAOptimal <= amountADesired)
l273 assert(wcanto.transfer(pair, amountCANTO))
l419 assert(wcanto.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]))

TOOLS USED

Manual Analysis

MITIGATION

Replace the assert statements with a require statement or a custom error

CloseFactor unbounded

PROBLEM

In Comptroller.sol, it is mentioned that closeFactorMantissa should be greater than closeFactorMinMantissa and less than closeFactorMaxMantissa. But in _setCloseFactor, these are not checked, meaning closeFactorMantissa can be set to a value outside the boundaries defined by the protocol.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

lending-market/Comptroller.sol

l81-l85
// closeFactorMantissa must be strictly greater than this value
uint internal constant closeFactorMinMantissa = 0.05e18; // 0.05

// closeFactorMantissa must not exceed this value
uint internal constant closeFactorMaxMantissa = 0.9e18; // 0.9
l850-l859
function _setCloseFactor(uint newCloseFactorMantissa) external returns (uint) {
    // Check caller is admin
    require(msg.sender == admin, "only admin can set close factor");

    uint oldCloseFactorMantissa = closeFactorMantissa;
    closeFactorMantissa = newCloseFactorMantissa;
    emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa);

    return uint(Error.NO_ERROR);
}

TOOLS USED

Manual Analysis

MITIGATION

Add checks in _setCloseFactor to ensure closeFactorMantissa is greater than closeFactorMinMantissa and less than closeFactorMaxMantissa

hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-periphery.sol

pair = address(uint160(uint256(keccak256(abi.encodePacked(
    hex'ff',
    factory,
    keccak256(abi.encodePacked(token0, token1, stable)), @audit this is a bytes type argument
    pairCodeHash // init code hash
)))))

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-core.sol

l107 (token0, token1, stable) = (_token0, _token1, _stable)

stableswap/BaseV1-periphery.sol

l75factory = _factory;
pairCodeHash = IBaseV1Factory(_factory).pairCodeHash();
wcanto = IWCANTO(_wcanto);

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the immutable variables aforementioned.

Initialize can be called more than once

IMPACT

in AccountantDelegate and TreasuryDelegate, the initialize() function has no check to make sure it has not been called before. This means a malicious admin can call these functions more than once and change the note and Cnote token contracts used.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

lending-market/AccountantDelegate.sol

l15 function initialize

lending-market/TreasuryDelegate.sol

l15 function initialize

TOOLS USED

Manual Analysis

MITIGATION

Add a require statement or modifier to ensure initialize() can only be called once.

Race conditions using old approve function

PROBLEM

The old approve() method of managing allowances has a race condition issue. Users of this token will be open to front-running attacks.

SEVERITY

Low

PROOF OF CONCEPT

lending-market/WETH.sol

l91 function _approve(
        address owner,
        address spender,
        uint256 amount
) internal   {
    require(owner != address(0), "ERC20: approve from the zero address");
    require(spender != address(0), "ERC20: approve to the zero address");

    _allowance[owner][spender] = amount;
    emit Approval(owner, spender, amount);
}

TOOLS USED

Manual Analysis

MITIGATION

Use an increase/decrease allowance type of methods instead.

Receive function

PROBLEM

AccountantDelegate has a receive() function, but does not have any withdrawal function. Any Manifest mistakenly sent to this contract would be locked.

SEVERITY

Low

PROOF OF CONCEPT

lending-market/AccountantDelegate.sol

l94 receive() external override payable {}

TOOLS USED

Manual Analysis

MITIGATION

Add require(0 == msg.value) in receive() or remove the function altogether.

Setters should check the input value

PROBLEM

Setters and initializers should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l144 function _setPendingAdmin()

lending-market/Comptroller.sol

l826 function _setPriceOracle()
l1015 function _setBorrowCapGuardian()
l1033 function _setPauseGuardian()
l1394 function _grantComp() //not a setter, but distribution function so should also check input address
l1423 function  _setContributorCompSpeed()

lending-market/Comptroller.sol

l15 function initialize()

lending-market/AccountantDelegate.sol

l20 function initialize() // treasury

lending-market/AccountantDelegator.sol

l35 constructor // admin

lending-market/TreasuryDelegate.sol

l46 function sendFund()

lending-market/TreasuryDelegator.sol

l21 constructor // admin

lending-market/CNote.sol

l14 function _setAccountantContract

stableswap/BaseV1-core.sol

l497 function setPauser()

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks - address or uint256 - to the setters aforementioned.

Transfer should check recipient not address zero

PROBLEM

ERC20 token implementations typically include zero-address checks on both sender and recipient addresses of transfer functions. This is not the case in WETH.sol, where no check is performed on the recipient

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l65-l83
function transferFrom(address src, address dst, uint wad)
    public
    returns (bool)
{
    require(_balanceOf[src] >= wad);

    if (src != msg.sender && _allowance[src][msg.sender] != type(uint).max) {
        require(_allowance[src][msg.sender] >= wad);
        _allowance[src][msg.sender] -= wad;
    }

    _balanceOf[src] -= wad;
    _balanceOf[dst] += wad;

    emit Transfer(src, dst, wad);

    return true;

}

TOOLS USED

Manual Analysis

MITIGATION

Add a zero-address check on dst

Underflow desired but not possible

PROBLEM

Underflow is desired in several price update functions of stableswap/BaseV1Pair, but as overflow/underflow checks are automatically performed since Solidity 0.8.0, the functions currently revert if there is underflow

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-core.sol

l156 uint timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
l183 uint timeElapsed = blockTimestamp - _blockTimestampLast

TOOLS USED

Manual Analysis

MITIGATION

Place these statements in an unchecked block to allow underflow

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l452 @param borrowerIndex
l526 @param seizeTokens
l677 @param accounts
l689 @param accounts
l826 @param newOracle
l1210 @param marketBorrowIndex
l1270 @param marketBorrowIndex

lending-market/CNote.sol

l31 @param borrower

lending-market/NoteInterest.sol

l92 @param cash
l92 @param borrows
l92 @param reserves
l109 @param cash
l109 @param borrows
l109 @param reserves
l109 @param reserveFactorMantissa

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l24 /* emit Deposit(msg.sender, msg.value); */
l32 /* emit Withdrawal(msg.sender, wad); */

lending-market/CNote.sol

l167 //comptroller.repayBorrowVerify(address(this), payer, borrower, vars.actualRepayAmount, vars.borrowerIndex)

stableswap/BaseV1-core.sol

l362 //amountIn -= amountIn / 10000; // remove fee from amount received

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/NoteInterest.sol

l95 100
l96 100

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Constructor visibility

PROBLEM

Visibility (public / external) is not needed for constructors anymore since Solidity 0.7.0, see here

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/AccountantDelegator.sol

l16 constructor(
            address implementation_,
            address admin_,
      address cnoteAddress_,
      address noteAddress_,
      address comptrollerAddress_, 
      address treasury_) public

TOOLS USED

Manual Analysis

MITIGATION

Remove the public modifier from constructors.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CNote.sol

emit Redeem(redeemer, redeemAmount, redeemTokens);

/* We call the defense hook */
comptroller.redeemVerify(address(this), redeemer, redeemAmount, redeemTokens)

TOOLS USED

Manual Analysis

MITIGATION

Place the defense hook before the two event emissions.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/Comptroller.sol

l19 event MarketListed(CToken cToken)
l22 event MarketEntered(CToken cToken, address account)
l25 event MarketExited(CToken cToken, address account)
l28 event NewCloseFactor(uint oldCloseFactorMantissa, uint newCloseFactorMantissa)
l31 event NewCollateralFactor(CToken cToken, uint oldCollateralFactorMantissa, uint newCollateralFactorMantissa)
l34 event NewLiquidationIncentive(uint oldLiquidationIncentiveMantissa, uint newLiquidationIncentiveMantissa)
l37 event NewPriceOracle(PriceOracle oldPriceOracle, PriceOracle newPriceOracle)
l40 event NewPauseGuardian(address oldPauseGuardian, address newPauseGuardian)
l43 event ActionPaused(string action, bool pauseState)
l46 event ActionPaused(CToken cToken, string action, bool pauseState)
l49 event CompBorrowSpeedUpdated(CToken indexed cToken, uint newSpeed)
l52 event CompSupplySpeedUpdated(CToken indexed cToken, uint newSpeed)
l55 event ContributorCompSpeedUpdated(address indexed contributor, uint newSpeed)
l58 event DistributedSupplierComp(CToken indexed cToken, address indexed supplier, uint compDelta, uint compSupplyIndex)
l61 event DistributedBorrowerComp(CToken indexed cToken, address indexed borrower, uint compDelta, uint compBorrowIndex)
l64 event NewBorrowCap(CToken indexed cToken, uint newBorrowCap)
l67 event NewBorrowCapGuardian(address oldBorrowCapGuardian, address newBorrowCapGuardian)
l70 event CompGranted(address recipient, uint amount)
l73 event CompAccruedAdjusted(address indexed user, uint oldCompAccrued, uint newCompAccrued)
l76 event CompReceivableUpdated(address indexed user, uint oldCompReceivable, uint newCompReceivable)

lending-market/AccountantInterfaces.sol

l15 event AcctInit(address lendingMarketAddress)
l16 event AcctSupplied(uint amount, uint err)
l25 event NewImplementation(address oldImplementation, address newImplementation)

lending-market/TreasuryInterfaces.sol

l17 event NewImplementation(address oldImplementation, address newImplementation)

lending-market/CNote.sol

l10 event AccountantSet(address accountant, address accountantPrior)

lending-market/NoteInterest.sol

l17 event NewInterestParams(uint baserateperblock)
l61 event NewBaseRate(uint oldBaseRateMantissa, uint newBaseRateMantissa)
l64 event NewAdjusterCoefficient(uint oldAdjusterCoefficient, uint newAdjusterCoefficient)
l67 event NewUpdateFrequency(uint oldUpdateFrequency, uint newUpdateFrequency)

stableswap/BaseV1-core.sol

l88 event Mint(address indexed sender, uint amount0, uint amount1);
l89 event Burn(address indexed sender, uint amount0, uint amount1, address indexed to);
l90 event Swap(
        address indexed sender,
        uint amount0In,
        uint amount1In,
        uint amount0Out,
        uint amount1Out,
        address indexed to
    );
l98 event Sync(uint reserve0, uint reserve1);
l99 event Claim(address indexed sender, address indexed recipient, uint amount0, uint amount1);
l101 event Transfer(address indexed from, address indexed to, uint amount);
l102 event Approval(address indexed owner, address indexed spender, uint amount)
l486 event PairCreated(address indexed token0, address indexed token1, bool stable, address pair, uint)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l22 function deposit() 
l28 function withdraw()

lending-market/GovernorBravoDelegate.sol

l131 function _initiate()

stableswap/BaseV1-core.sol

l497 function setPauser()
l507 function setPause()

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function missing comments

PROBLEM

Some functions are missing Natspec comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

manifest/Proposal-Store.sol

l46 function AddProposal
l52 function QueryProp

lending-market/WETH.sol

All the functions are missing comments

lending-market/GovernorBravoDelegate.sol

l77 function queueOrRevertInternal
l180 function add256
l186 function sub256
l191 function getChainIdInternal()

lending-market/Comptroller.sol

l294 function redeemAllowedInternal
l180 function add256
l186 function sub256
l191 function getChainIdInternal()
l958 function _addMarketInternal()
l965 function _initializeMarket
l1050 function _setMintPaused
l1060 function _setBorrowPaused
l1070 function _setTransferPaused
l1079 function _setSeizePaused
l1088 function _become
l1094 function fixBadAccruals
l1144 function adminOrInitializing
l1461 function getBlockNumber

lending-market/CNote.sol

l14 function _setAccountantContract
l23 function getAccountant

stableswap/BaseV1-core.sol

All the functions are missing proper Natspec comments.

stableswap/BaseV1-periphery.sol

All the functions are missing proper Natspec comments.

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Function order

PROBLEM

Functions should be ordered following the Soldiity conventions: receive() function should be placed after the constructor and before every other function.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Several contracts have receive() and fallback() at the end:

TOOLS USED

Manual Analysis

MITIGATION

Place the receive() and fallback() functions after the constructor, before all the other functions.

Local variable shadowing

IMPACT

In lending-market/NoteInterest.sol, there is local variable shadowing: the constructor parameter has the same name as the storage variable baseRatePerYear. This will not lead to any error but can be confusing, especially in the constructor where baseRatePerBlock is computed using the constructor parameter baseRatePerYear.

SEVERITY

Non-critical

PROOF OF CONCEPT

Instances include:

lending-market/NoteInterest.sol

constructor(uint baseRatePerYear) {
    baseRatePerBlock = baseRatePerYear.div(blocksPerYear)

TOOLS USED

Manual Analysis

MITIGATION

Add an underscore to the constructor parameter (_baseRatePerYear) to avoid shadowing.

Non-library files should use fixed compiler versions

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ZoneInteraction.sol

WETH.sol, GovernorBravoDelegate.sol, Comptroller.sol, AccountantDelegate.sol, AccountantDelegator.sol, AccountantInterfaces.sol, TreasuryDelegate.sol, TreasuryDelegator.sol, TreasuryInterfaces.sol, CNote.sol and NoteInterest.sol have floating pragmas.

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

Non-library files should use the same compiler version

PROBLEM

contracts within the scope should be compiled using the same compiler version.

SEVERITY

Non-Critical

PROOF OF CONCEPT

WETH.sol, GovernorBravoDelegate.sol, Comptroller.sol, AccountantDelegate.sol, AccountantDelegator.sol, AccountantInterfaces.sol, TreasuryDelegate.sol, TreasuryDelegator.sol, TreasuryInterfaces.sol, CNote.sol and NoteInterest.sol have the compiler version set to ^0.8.10, while BaseV1-core.sol and BaseV1-periphery have the 0.8.11 version.

TOOLS USED

Manual Analysis

MITIGATION

Use the same compiler version throughout the contracts

open TODOs

PROBLEM

There are open TODOs in the code. Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/Comptroller.sol

l1232 // TODO: Don't distribute supplier COMP if the user is not in the supplier market.
l1271 // TODO: Don't distribute supplier COMP if the user is not in the borrower market.

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

manifest/Proposal-Store.sol

l46 function AddProposal()
l52 function QueryProp()

lending-market/GovernorBravoDelegate.sol

l24 function initialize()

lending-market/Comptroller.sol

l122 function enterMarkets()
l677 function getAccountLiquidity()
l703 function getHypotheticalAccountLiquidity()
l826 function _setPriceOracle()
l1033 function _setPauseGuardian()
l1050 function _setMintPaused()
l1060 function _setBorrowPaused()
l1070 function _setTransferPaused()
l1079 function _setSeizePaused()
l1088 function _become()
l1324 function claimComp(address holder)
l1394 function _grantComp()
l1407 function _setCompSpeeds()
l1423 function _setContributorCompSpeed()
l1444 function getAllMarkets()

lending-market/AccountantDelegate.sol

l15 function initialize()

lending-market/AccountantDelegator.sol

l109 delegateToViewImplementation()

lending-market/TreasuryDelegate.sol

l15 function initialize()

lending-market/TreasuryDelegator.sol

l84 delegateToViewImplementation()

lending-market/CNote.sol

l14 function _setAccountantContract

lending-market/NoteInterest.sol

l118 function updateBaseRate

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-core.sol

l85 mapping(address => uint) public supplyIndex0
l86 mapping(address => uint) public supplyIndex1

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping:

struct SupplyIndex {
  uint256 index0;
  uint256 index1;
}

And it would be used as a state variable:

mapping(address =>  SupplyIndex) public supplyIndexes;

Require statements should have descriptive strings

PROBLEM

Some require statements are missing error strings, which makes it more difficult to debug when the function reverts.

SEVERITY

Non-critical

PROOF OF CONCEPT

lending-market/WETH.sol

l69 require(_balanceOf[src] >= wad)
l72 require(_allowance[src][msg.sender] >= wad)

lending-market/GovernorBravoDelegate.sol

l53 require(proposals[unigovProposal.id].id == 0)

stableswap/BaseV1-core.sol

l125 require(_unlocked == 1)
l285 require(!BaseV1Factory(factory).isPaused());
l465 require(token.code.length > 0)
l468 require(success && (data.length == 0 || abi.decode(data, (bool))))
l498 require(msg.sender == pauser)
l503 require(msg.sender == pendingPauser)
l508 require(msg.sender == pauser)

stableswap/BaseV1-periphery.sol

l210 require(amountADesired >= amountAMin);
l211 require(amountBDesired >= amountBMin)
l291 require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity))
l456 require(token.code.length > 0)
l459 require(success && (data.length == 0 || abi.decode(data, (bool))))
l463 require(token.code.length > 0, "token code length faialure")
l466 require(success && (data.length == 0 || abi.decode(data, (bool))), "failing here")

TOOL USED

Manual Analysis

MITIGATION

Add error strings to all require statements.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-periphery.sol

l67 uint internal constant MINIMUM_LIQUIDITY = 10**3

TOOLS USED

Manual Analysis

MITIGATION

Replace 10**3 with 10e3

Styling

PROBLEM

There should be space between operands in mathematical computations

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-periphery.sol

l134 routes.length+1
l139 amounts[i+1]
l366 routes[i+1].from, routes[i+1].to, routes[i+1].stable

TOOLS USED

Manual Analysis

MITIGATION

Add spaces, e.g

-routes.length+1
+routes.length + 1

Typos

PROBLEM

There are a few typos in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

lending-market/NoteInterest.sol

l89 irrelevent

stableswap/BaseV1-periphery.sol

l463 faialure

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

All the contracts in scope use uint instead of uint256

TOOLS USED

Manual Analysis

MITIGATION

replace uint with uint256

Update Solidity version

IMPACT

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked()

SEVERITY

Non-Critical

PROOF OF CONCEPT

All the contracts in scope have a Solidity compiler version <0.8.12, and string.concat could be used in the following location:

stableswap/BaseV1-core.sol

l109 name = string(abi.encodePacked("StableV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
 symbol = string(abi.encodePacked("sAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
} else {
    name = string(abi.encodePacked("VolatileV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
    symbol = string(abi.encodePacked("vAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()))

TOOLS USED

Manual Analysis

MITIGATION

Use Solidity 0.8.12 and replace string(abi.encodePacked(..) with string.concat()

GalloDaSballo commented 2 years ago

L01 - assert statement should not be used

Valid L

L02 - CloseFactor unbounded

L -> May bump up

hash collision with abi.encodePacked

Disagree because you'd need to be able to create a second pair with the same addresses, which you cannot

L03 - Immutable addresses lack zero-address check

Valid L

Initialize can be called more than once

Disagree as once note is non-zero you cannot initialize

GalloDaSballo commented 2 years ago

Race conditions using old approve function

Disputed that's on the caller to be aware not on the token dev

L04 Receive function

Valid Low

Setters should check the input value

Valid Bulked with the zero check above

GalloDaSballo commented 2 years ago

L05 - Local variable shadowing

L

NC01 - Underflow desired but not possible

I ran the math on a previous contest and it would take longer for the sun to extinguish than the overflow to happen, for that reason NC

NC02 -Comment Missing function parameter

NC

NC03 - Constants instead of magic numbers

R

NC04 - Constructor visibility

NC

Events emitted early

Disputed as Slither will give false positives and some devs do that as mitigation

NC05 - Events indexing

NC

NC06 - Event should be emitted in setters

NC

NC07 - Function missing comments

NC

 NC08 - Function order

R

NC09 - Non-library files should use fixed compiler versions

NC

NC10 - open TODOs

NC

NC11 - Public functions can be external

R

Structs

Dispute in lack of details

NC12 - Require statements should have descriptive strings

NC

NC13 - Scientific notation

R

NC14 - Styling

NC

NC15 - Typos

NC

Rest I disagree

Overall this report feels like a dump of regex based queries

GalloDaSballo commented 2 years ago

5 L 4 R 11 NC

GalloDaSballo commented 2 years ago

Headings for report

L01 - assert statement should not be used

L02 - CloseFactor unbounded

L03 - Immutable addresses lack zero-address check

L04 Receive function

L05 - Local variable shadowing

L06 - AVOID USING .TRANSFER TO TRANSFER NATIVE TOKENS (#142)

NC01 - Underflow desired but not possible

NC02 -Comment Missing function parameter

NC03 - Constants instead of magic numbers

NC04 - Constructor visibility

NC05 - Events indexing

NC06 - Event should be emitted in setters

NC07 - Function missing comments

NC08 - Function order

NC09 - Non-library files should use fixed compiler versions

NC10 - open TODOs

NC11 - Public functions can be external

NC12 - Require statements should have descriptive strings

NC13 - Scientific notation

NC14 - Styling

NC15 - Typos