Canto contest - joestakey's results

Execution layer for original work.

General Information

Platform: Code4rena

Start Date: 14/06/2022

Pot Size: $100,000 USDC

Total HM: 26

Participants: 59

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 9

Id: 133

League: ETH

Canto

Findings Distribution

Researcher Performance

Rank: 23/59

Findings: 2

Award: $951.63

🌟 Selected for report: 1

🚀 Solo Findings: 0

Awards

687.9945 CANTO - $111.11

708.3931 USDC - $708.39

Labels

bug
QA (Quality Assurance)

External Links

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:

  • lending-market/AccountantDelegate.sol

  • lending-market/AccountantDelegator.sol

  • lending-market/TreasuryDelegator.sol

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

#0 - GalloDaSballo

2022-08-03T00:59:44Z

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

#1 - GalloDaSballo

2022-08-03T23:11:56Z

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

#2 - GalloDaSballo

2022-08-03T23:19:37Z

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

#3 - GalloDaSballo

2022-08-03T23:20:38Z

5 L 4 R 11 NC

#4 - GalloDaSballo

2022-08-16T13:43:29Z

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

Awards

396.9199 CANTO - $64.10

68.0285 USDC - $68.03

Labels

bug
G (Gas Optimization)

External Links

Gas Report

Table of Contents

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

scope: queue()

  • newProposal.targets.length is read newProposal.targets.length times
l68: i < newProposal.targets.length

scope: queueOrRevertInternal()

  • timelock is read twice
l78: !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))) l79: timelock.queueTransaction(target, value, signature, data, eta)

scope: execute()

  • proposal.targets.length is read proposal.targets.length times
l90: i < proposal.targets.length

scope: _acceptAdmin()

  • pendingAdmin is read 3 times
l164: msg.sender == pendingAdmin l168 address oldPendingAdmin = pendingAdmin l171 admin = pendingAdmin

lending-market/Comptroller.sol

scope: queue()

  • allMarkets.length is read allMarkets.length times
l959 i < allMarkets.length

scope: _setBorrowPaused()

  • admin is read twice
l1062 msg.sender == admin l1063 msg.sender == admin

scope: _setTransferPaused()

  • admin is read twice
l1071 msg.sender == admin l1072 msg.sender == admin

scope: _setSeizePaused()

  • admin is read twice
l1080 msg.sender == admin l1081 msg.sender == admin

lending-market/AccountantDelegate.sol

scope: initialize()

  • note is read 4 times
l28 note._mint_to_Accountant(msg.sender) l29 require(note.balanceOf(msg.sender) == note._initialSupply() l39 note.approve(cnoteAddress_, type(uint).max)

scope: supplyMarket()

  • cnote is read twice
l48 require(msg.sender == address(cnote) l49 cnote.mint(amount)

scope: redeemMarket()

  • cnote is read 3 times
l60 require(msg.sender == address(cnote) l62 Exp({mantissa: cnote.exchangeRateStored()}) l66 cnote.redeem(amtToRedeem

scope: sweepInterest()

  • note is read 3 times
l76 note.balanceOf(address(this)) l81 sub_(note.totalSupply(), noteBalance) l87 note.transfer(treasury, amtToSweep)
  • cnote is read 3 times
l77 cnote.balanceOf(address(this)) l79 Exp({mantissa: cnote.exchangeRateStored()}) l89 cnote.transfer(address(0), CNoteBalance)

lending-market/cnote.sol

scope: _setAccountantContract()

  • _accountant is read twice
l15 address(_accountant) l18 emit AccountantSet(accountant_, address(_accountant))

scope: borrowFresh()

  • _accountant is read twice
l45 address(_accountant) l48 _accountant.supplyMarket(borrowAmount)

scope: repayBorrowFresh()

  • _accountant is read twice
l134 _accountant.redeemMarket(amtRedeemed) l141 payable(_accountant)

scope: mintFresh()

  • _accountant is read 3 times
l179 address(_accountant) l180 address(_accountant) l225 payable(_accountant)

scope: redeemFresh()

  • _accountant is read 3 times
l259 address(_accountant) l260 payable(_accountant) l324 _accountant.supplyMarket(redeemAmount)
  • comptroller is read twice
l294 comptroller.redeemAllowed(address(this), redeemer, redeemTokens) l343 comptroller.redeemVerify(address(this), redeemer, redeemAmount, redeemTokens)

stableswap/BaseV1-core.sol

scope: acceptPauser()

  • pendingPauser is read twice
l503 require(msg.sender == pendingPauser); l504 pauser = pendingPauser

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

scope: queueOrRevertInternal

l77 string memory signature, bytes memory data

lending-market/AccountantDelegator.sol

scope: delegateTo

l82 bytes memory data

scope: delegateToImplementation

l98 bytes memory data

scope: delegateToViewImplementation

l109 bytes memory data

lending-market/TreasuryDelegator.sol

scope: delegateToImplementation

73 bytes memory data

scope: delegateToViewImplementation

l84 bytes memory data

scope: delegateTo

l101 bytes memory data

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas, approximately 20 gas in require and if statements

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l29: _balanceOf[msg.sender] >= wad l72: _balanceOf[src] >= wad l29: _allowance[src][msg.sender] >= wad

lending-market/GovernorBravoDelegate.sol

l47 require(unigovProposal.targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions") l115 proposalCount >= proposalId l119 block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) l182 require(c >= a, "addition overflow") l187 require(b <= a, "subtraction underflow")

lending-market/Comptroller.sol

l491 borrowBalance >= repayAmount l1243 supplyIndex >= compInitialIndex l1282 borrowIndex >= compInitialIndex l1379 amount <= compRemaining

lending-market/AccountantDelegate.sol

l83 cNoteConverted >= noteDifferential

lending-market/CNote.sol

l130 getCashPrior() >= actualRepayAmount

stableswap/BaseV1-core.sol

l309 _k(_balance0, _balance1) >= _k(_reserve0, _reserve1), 'K' l348 y - y_prev <= 1 l352 y_prev - y <= 1 l413 deadline >= block.timestamp

stableswap/BaseV1-periphery.sol

l71 deadline >= block.timestamp l133 routes.length >= 1 l169 amountBOptimal <= amountBDesired l210 amountADesired >= amountAMin l211 amountBDesired >= amountBMin l222 amountBOptimal <= amountBDesired l223 amountBOptimal >= amountBMin l227 amountAOptimal <= amountADesired l228 amountAOptimal >= amountAMin l295 amountA >= amountAMin l296 amountB >= amountBMin l387 amounts[amounts.length - 1] >= amountOutMin l402 amounts[amounts.length - 1] >= amountOutMin l417 amounts[amounts.length - 1] >= amountOutMin l430 amounts[amounts.length - 1] >= amountOutMin

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-proposalCount >= proposalId; +proposalCount > proposalId - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

Constant expressions

IMPACT

Constant expressions are re-calculated each time it is in use, costing an extra 97 gas than a constant every time they are called.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l15 bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)") l18 bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)")

TOOLS USED

Manual Analysis

MITIGATION

Mark these as immutable instead of constant

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l14: _name = name_; l15: _symbol = symbol_;

stableswap/BaseV1-core.sol

l490 isPaused = false //false is the default value of isPaused, this line can be removed altogether

stableswap/BaseV1-periphery.sol

l76 factory = _factory l77 pairCodeHash = IBaseV1Factory(_factory).pairCodeHash() l78 wcanto = IWCANTO(_wcanto)

TOOLS USED

Manual Analysis

MITIGATION

Hardcode storage variables with their initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here.

It not only saves gas upon deployment - ~5500 gas saved per custom error instead of a require statement, but it is also cheaper in a function call, 22 gas saved per require statement replaced with a custom error.

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l29 require(_balanceOf[msg.sender] >= wad, "sender balance insufficient for withdrawal") l69 require(_balanceOf[src] >= wad) l72 require(_allowance[src][msg.sender] >= wad) l96 require(owner != address(0), "ERC20: approve from the zero address") l97 require(spender != address(0), "ERC20: approve to the zero address")

lending-market/GovernorBravoDelegate.sol

l25 require(address(timelock) == address(0), "GovernorBravo::initialize: can only initialize once"); l26 require(msg.sender == admin, "GovernorBravo::initialize: admin only"); l27 require(timelock_ != address(0), "GovernorBravo::initialize: invalid timelock address") l42 require(unigovProposal.targets.length == unigovProposal.values.length && unigovProposal.targets.length == unigovProposal.signatures.length && unigovProposal.targets.length == unigovProposal.calldatas.length, "GovernorBravo::propose: proposal function information arity mismatch") l46 require(unigovProposal.targets.length != 0, "GovernorBravo::propose: must provide actions"); l47 require(unigovProposal.targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions") l53 require(proposals[unigovProposal.id].id == 0) l78 require(!timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))), "GovernorBravo::queueOrRevertInternal: identical proposal action already queued at eta") l87 require(state(proposalId) == ProposalState.Queued, "GovernorBravo::execute: proposal can only be executed if it is queued") l115 require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id") l132 require(msg.sender == admin, "GovernorBravo::_initiate: admin only") l133 require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once") l146 require(msg.sender == admin, "GovernorBravo:_setPendingAdmin: admin only") l164 require(msg.sender == pendingAdmin && msg.sender != address(0), "GovernorBravo:_acceptAdmin: pending admin only") l182 require(c >= a, "addition overflow") l187 require(b <= a, "subtraction underflow")

lending-market/Comptroller.sol

l178 require(oErr == 0, "exitMarket: getAccountSnapshot failed") l237 require(!mintGuardianPaused[cToken], "mint is paused") l343 require(!borrowGuardianPaused[cToken], "borrow is paused") l351 require(msg.sender == cToken, "sender must be cToken") l373 require(nextTotalBorrows < borrowCap, "market borrow cap reached") l491 require(borrowBalance >= repayAmount, "Can not repay more than the total borrow") l556 require(!seizeGuardianPaused, "seize is paused") l614 require(!transferGuardianPaused, "transfer is paused") l852 require(msg.sender == admin, "only admin can set close factor") l960 require(allMarkets[i] != CToken(cToken), "market already added") l998 require(msg.sender == admin || msg.sender == borrowCapGuardian, "only admin or borrow cap guardian can set borrow caps") l1003 require(numMarkets != 0 && numMarkets == numBorrowCaps, "invalid input") l1016 require(msg.sender == admin, "only admin can set borrow cap guardian") l1051 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed"); l1052 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1053 require(msg.sender == admin || state == true, "only admin can unpause") l1061 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed"); l1062 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1063 require(msg.sender == admin || state == true, "only admin can unpause") l1071 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1072 require(msg.sender == admin || state == true, "only admin can unpause") l1080 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1081 require(msg.sender == admin || state == true, "only admin can unpause") l1089 require(msg.sender == unitroller.admin(), "only unitroller admin can change brains"); l1090 require(unitroller._acceptImplementation() == 0, "change not authorized"); l1095 require(msg.sender == admin, "Only admin can call this function"); // Only the timelock can call this function l1096 require(!proposal65FixExecuted, "Already executed this one-off function"); // Require that this function is only called once l1097 require(affectedUsers.length == amounts.length, "Invalid input") l1158 require(market.isListed, "comp market is not listed") l1349 require(markets[address(cToken)].isListed, "market must be listed") l1395 require(adminOrInitializing(), "only admin can grant comp") l1397 require(amountLeft == 0, "insufficient comp for grant") l1408 require(adminOrInitializing(), "only admin can set comp speed") l1411 require(numTokens == supplySpeeds.length && numTokens == borrowSpeeds.length, "Comptroller::_setCompSpeeds invalid input") l1424 require(adminOrInitializing(), "only admin can set comp speed")

lending-market/AccountantDelegate.sol

l17 require(msg.sender == admin, "AccountantDelegate::initialize: only admin can call this function"); l18 require(noteAddress_ != address(0), "AccountantDelegate::initialize: note Address invalid") l29 require(note.balanceOf(msg.sender) == note._initialSupply(), "AccountantDelegate::initiatlize: Accountant has not received payment") l48 require(msg.sender == address(cnote), "AccountantDelegate::supplyMarket: Only the CNote contract can supply market") l60 require(msg.sender == address(cnote), "AccountantDelegate::redeemMarket: Only the CNote contract can redeem market") l83 require(cNoteConverted >= noteDifferential, "Note Loaned to LendingMarket must increase in value")

lending-market/AccountantDelegator.sol

l43 require(msg.sender == admin, "AccountantDelegator::_setImplementation: admin only") l44 require(implementation_ != address(0), "AccountantDelegator::_setImplementation: invalid implementation address") l124 require(msg.value == 0,"AccountantDelegator:fallback: cannot send value to fallback")

lending-market/TreasuryDelegator.sol

l31 require(msg.sender == admin, "GovernorBravoDelegator::_setImplementation: admin only") l32 require(implementation_ != address(0), "GovernorBravoDelegator::_setImplementation: invalid implementation address")

lending-market/CNote.sol

l16 require(msg.sender == admin, "CNote::_setAccountantContract:Only admin may call this function") l43 require(getCashPrior() == 0, "CNote::borrowFresh:Impossible reserves in CNote market Place") l45 require(address(_accountant) != address(0), "CNote::borrowFresh:Accountant has not been initialized") l54 require(getCashPrior() == borrowAmount, "CNote::borrowFresh:Error in Accountant supply") l77 require(getCashPrior() == 0,"CNote::borrowFresh: Error in doTransferOut, impossible Liquidity in LendingMarket") l114 require(getCashPrior() == 0, "CNote::repayBorrowFresh:Liquidity in Note Lending Market is always flashed") l130 require(getCashPrior() >= actualRepayAmount, "CNote::repayBorrowFresh: doTransferIn supplied incorrect amount") l146 require(getCashPrior() == 0, "CNote::repayBorrowFresh: Error in Accountant.redeemMarket") l198 require(getCashPrior() == 0, "CNote::mintFresh: Any Liquidity in the Lending Market is flashed") l214 require(getCashPrior() == actualMintAmount, "CNote::mintFresh: Error in doTransferIn, CNote reserves >= mint Amount") l229 require(getCashPrior() == 0) l264 require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero") l310 require(getCashPrior() == 0, "CNote::redeemFresh, LendingMarket has > 0 Cash before Accountant Supplies") l330 require(getCashPrior() == redeemAmount, "CNote::redeemFresh: Accountant has supplied incorrect Amount") l353 require(_notEntered, "re-entered")

stableswap/BaseV1-core.sol

l125 require(_unlocked == 1) l253 require(liquidity > 0, 'ILM') l272 require(amount0 > 0 && amount1 > 0, 'ILB') l285 require(!BaseV1Factory(factory).isPaused()); l286 require(amount0Out > 0 || amount1Out > 0, 'IOA') l288 require(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL') l294 require(to != _token0 && to != _token1, 'IT') l303 require(amount0In > 0 || amount1In > 0, 'IIA') l309 require(_k(_balance0, _balance1) >= _k(_reserve0, _reserve1), 'K') l413 require(deadline >= block.timestamp, 'BaseV1: EXPIRED') l431 require(recoveredAddress != address(0) && recoveredAddress == owner, 'BaseV1: INVALID_SIGNATURE') 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) l521 require(tokenA != tokenB, 'IA') l523 require(token0 != address(0), 'ZA') l524 require(getPair[token0][token1][stable] == address(0), 'PE')

stableswap/BaseV1-periphery.sol

l71 require(deadline >= block.timestamp, 'BaseV1Router: EXPIRED') l86 require(tokenA != tokenB, 'BaseV1Router: IDENTICAL_ADDRESSES') l88 require(token0 != address(0), 'BaseV1Router: ZERO_ADDRESS') l104 require(amountA > 0, 'BaseV1Router: INSUFFICIENT_AMOUNT') l105 require(reserveA > 0 && reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY') l133 require(routes.length >= 1, 'BaseV1Router: INVALID_PATH') l210 require(amountADesired >= amountAMin); l211 require(amountBDesired >= amountBMin) l223 require(amountBOptimal >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT') l228 require(amountAOptimal >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT') l291 require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity)) l295 require(amountA >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT'); l296 require(amountB >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT') l387 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l402 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l415 require(routes[0].from == address(wcanto), 'BaseV1Router: INVALID_PATH') l417 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l428 require(routes[routes.length - 1].to == address(wcanto), 'BaseV1Router: INVALID_PATH') l430 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l452 require(success, 'TransferHelper: ETH_TRANSFER_FAILED') 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")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in lending-market/GovernorBravoDelegate.sol:

Replace

require(address(timelock) == address(0), "GovernorBravo::initialize: can only initialize once")

with

if (address(timelock) != address(0)) { revert IsInitialized(); }

and define the custom error in the contract

error IsInitialized();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes 22 gas per variable initialized.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l68 uint i = 0 l90 uint i = 0

lending-market/Comptroller.sol

l126 uint i = 0 l206 uint i = 0 l735 uint i = 0 l959 uint i = 0 l1005 uint i = 0 l1106 uint i = 0 l1347 uint i = 0 l1353 uint j = 0 l1359 uint j = 0 l1364 uint j = 0 l1413 uint i = 0

BaseV1-core.sol

l46 uint public totalSupply = 0 l207 uint i = 0 l223 uint nextIndex = 0 l224 uint index = 0 l337 uint i = 0

BaseV1-periphery.sol

l136 uint i = 0 l158 uint _totalSupply = 0 l362 uint i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l176 emit NewAdmin(oldAdmin, admin); l177 emit NewPendingAdmin(oldPendingAdmin, pendingAdmin)

lending-market/NoteInterest.sol

l75 emit NewInterestParams(baseRatePerBlock) l127 emit NewInterestParams(baseRatePerYear) l144 emit NewBaseRate(oldBaseRatePerYear, baseRatePerYear) l157 emit NewAdjusterCoefficient(oldAdjusterCoefficient, adjusterCoefficient) l170 emit NewUpdateFrequency(oldUpdateFrequency, updateFrequency)

stableswap/BaseV1-core.sol

l170 emit Sync(reserve0, reserve1)

TOOLS USED

Manual Analysis

MITIGATION

Emit a local variable or function argument instead of storage variable

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwrds, marking it as immutable can save a storage operation - 20,000 gas.

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l14: _name = name_; l15: _symbol = symbol_;

TOOLS USED

Manual Analysis

MITIGATION

Mark these variables as immutable.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y.

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l23 _balanceOf[msg.sender] += msg.value l30 _balanceOf[msg.sender] -= wad l73 _allowance[src][msg.sender] -= wad l76 _balanceOf[src] -= wad l77 _balanceOf[dst] += wad

stableswap/BaseV1-core.sol

l158 reserve0CumulativeLast += _reserve0 * timeElapsed; l159 reserve1CumulativeLast += _reserve1 * timeElapsed; l184 reserve0Cumulative += _reserve0 * timeElapsed; l185 reserve1Cumulative += _reserve1 * timeElapsed l208 priceAverageCumulative += _prices[i] l226 i+=window l394 totalSupply += amount l395 balanceOf[dst] += amount l400 totalSupply -= amount l401 balanceOf[dst] -= amount l458 balanceOf[src] -= amount l459 balanceOf[dst] += amount

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Modifier instead of duplicate require

PROBLEM

When a require statement is use multiple times, it is cheaper to use a modifier instead.

PROOF OF CONCEPT

Instances include:

lending-market/Comptroller.sol

l1051 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed") l1061 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed")
l1052 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause") l1062 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause") l1071 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause") l1080 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause")
l1053 require(msg.sender == admin || state == true, "only admin can unpause") l1063 require(msg.sender == admin || state == true, "only admin can unpause") l1072 require(msg.sender == admin || state == true, "only admin can unpause") l1081 require(msg.sender == admin || state == true, "only admin can unpause")
l1408 require(adminOrInitializing(), "only admin can set comp speed") l1424 require(adminOrInitializing(), "only admin can set comp speed")

stableswap/BaseV1-core.sol

l498 require(msg.sender == pauser) l508 require(msg.sender == pauser)

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments: it returns the incremented variable instead of returning a temporary variable storing the initial value of the variable. It saves 5 gas per iteration

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l68 i++ l90 i++

lending-market/Comptroller.sol

l126 i++ l206 i++ l735 i++ l959 i++ l1005 i++ l1106 i++ l1347 i++ l1353 j++ l1359 j++ l1364 j++

stableswap/BaseV1-core.sol

l207 i++ l337 i++

stableswap/BaseV1-periphery.sol

l136 i++ l362 i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l42 require(unigovProposal.targets.length == unigovProposal.values.length && unigovProposal.targets.length == unigovProposal.signatures.length && unigovProposal.targets.length == unigovProposal.calldatas.length, "GovernorBravo::propose: proposal function information arity mismatch") l115 require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id") l164 require(msg.sender == pendingAdmin && msg.sender != address(0), "GovernorBravo:_acceptAdmin: pending admin only")

lending-market/Comptroller.sol

l1003 require(numMarkets != 0 && numMarkets == numBorrowCaps, "invalid input") l1411 require(numTokens == supplySpeeds.length && numTokens == borrowSpeeds.length, "Comptroller::_setCompSpeeds invalid input")

stableswap/BaseV1-core.sol

l272 require(amount0 > 0 && amount1 > 0, 'ILB') l288 require(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL') l294 require(to != _token0 && to != _token1, 'IT') l431 require(recoveredAddress != address(0) && recoveredAddress == owner, 'BaseV1: INVALID_SIGNATURE') l468 require(success && (data.length == 0 || abi.decode(data, (bool))))

stableswap/BaseV1-periphery.sol

l105 require(reserveA > 0 && reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY') l459 require(success && (data.length == 0 || abi.decode(data, (bool)))) l466 require(success && (data.length == 0 || abi.decode(data, (bool))), "failing here")

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

-require(msg.sender == pendingAdmin && msg.sender != address(0), "GovernorBravo:_acceptAdmin: pending admin only") +require(msg.sender == pendingAdmin) +require(msg.sender != address(0))

You can also improve gas savings by using custom errors

Return statements

IMPACT

Named returns are the most gas efficient return statements, but there is no gas saving if the named return is unused and a return statement is used - costing an extra 2,000 gas per function call.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

l106 return (p.targets, p.values, p.signatures, p.calldatas)

stableswap/BaseV1-core.sol

l140 return (decimals0, decimals1, reserve0, reserve1, stable, token0, token1) l210 return priceAverageCumulative / granularity

stableswap/BaseV1-periphery.sol

l128 return amountStable > amountVolatile ? (amountStable, true) : (amountVolatile, false)

TOOLS USED

Manual Analysis

MITIGATION

Replace the return statements as explained, using a local variable with the named return instead.

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes. Each string exceeding that 32-byte size adds an extra 9,500 gas upon deployment.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include:

lending-market/WETH.sol

l29 sender balance insufficient for withdrawal l96 ERC20: approve from the zero address l97 ERC20: approve to the zero address

lending-market/GovernorBravoDelegate.sol

l25 require(address(timelock) == address(0), "GovernorBravo::initialize: can only initialize once"); l26 require(msg.sender == admin, "GovernorBravo::initialize: admin only"); l27 require(timelock_ != address(0), "GovernorBravo::initialize: invalid timelock address") l42 require(unigovProposal.targets.length == unigovProposal.values.length && unigovProposal.targets.length == unigovProposal.signatures.length && unigovProposal.targets.length == unigovProposal.calldatas.length, "GovernorBravo::propose: proposal function information arity mismatch") l46 require(unigovProposal.targets.length != 0, "GovernorBravo::propose: must provide actions"); l47 require(unigovProposal.targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions") l78 require(!timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))), "GovernorBravo::queueOrRevertInternal: identical proposal action already queued at eta") l87 require(state(proposalId) == ProposalState.Queued, "GovernorBravo::execute: proposal can only be executed if it is queued") l115 require(proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id") l132 require(msg.sender == admin, "GovernorBravo::_initiate: admin only") l133 require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once") l146 require(msg.sender == admin, "GovernorBravo:_setPendingAdmin: admin only") l164 require(msg.sender == pendingAdmin && msg.sender != address(0), "GovernorBravo:_acceptAdmin: pending admin only")

lending-market/Comptroller.sol

l178 require(oErr == 0, "exitMarket: getAccountSnapshot failed") l491 require(borrowBalance >= repayAmount, "Can not repay more than the total borrow") l998 require(msg.sender == admin || msg.sender == borrowCapGuardian, "only admin or borrow cap guardian can set borrow caps") l1016 require(msg.sender == admin, "only admin can set borrow cap guardian") l1051 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed"); l1052 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1061 require(markets[address(cToken)].isListed, "cannot pause a market that is not listed"); l1062 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1071 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1080 require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause"); l1089 require(msg.sender == unitroller.admin(), "only unitroller admin can change brains"); l1095 require(msg.sender == admin, "Only admin can call this function"); l1096 require(!proposal65FixExecuted, "Already executed this one-off function"); l1411 require(numTokens == supplySpeeds.length && numTokens == borrowSpeeds.length, "Comptroller::_setCompSpeeds invalid input")

lending-market/AccountantDelegate.sol

l17 require(msg.sender == admin, "AccountantDelegate::initialize: only admin can call this function"); l18 require(noteAddress_ != address(0), "AccountantDelegate::initialize: note Address invalid") l29 require(note.balanceOf(msg.sender) == note._initialSupply(), "AccountantDelegate::initiatlize: Accountant has not received payment") l48 require(msg.sender == address(cnote), "AccountantDelegate::supplyMarket: Only the CNote contract can supply market") l60 require(msg.sender == address(cnote), "AccountantDelegate::redeemMarket: Only the CNote contract can redeem market") l83 require(cNoteConverted >= noteDifferential, "Note Loaned to LendingMarket must increase in value")

lending-market/AccountantDelegator.sol

l43 require(msg.sender == admin, "AccountantDelegator::_setImplementation: admin only") l44 require(implementation_ != address(0), "AccountantDelegator::_setImplementation: invalid implementation address") l124 require(msg.value == 0,"AccountantDelegator:fallback: cannot send value to fallback")

lending-market/TreasuryDelegator.sol

l31 require(msg.sender == admin, "GovernorBravoDelegator::_setImplementation: admin only") l32 require(implementation_ != address(0), "GovernorBravoDelegator::_setImplementation: invalid implementation address")

lending-market/CNote.sol

l16 require(msg.sender == admin, "CNote::_setAccountantContract:Only admin may call this function") l43 require(getCashPrior() == 0, "CNote::borrowFresh:Impossible reserves in CNote market Place") l45 require(address(_accountant) != address(0), "CNote::borrowFresh:Accountant has not been initialized") l54 require(getCashPrior() == borrowAmount, "CNote::borrowFresh:Error in Accountant supply") l77 require(getCashPrior() == 0,"CNote::borrowFresh: Error in doTransferOut, impossible Liquidity in LendingMarket") l114 require(getCashPrior() == 0, "CNote::repayBorrowFresh:Liquidity in Note Lending Market is always flashed") l130 require(getCashPrior() >= actualRepayAmount, "CNote::repayBorrowFresh: doTransferIn supplied incorrect amount") l146 require(getCashPrior() == 0, "CNote::repayBorrowFresh: Error in Accountant.redeemMarket") l198 require(getCashPrior() == 0, "CNote::mintFresh: Any Liquidity in the Lending Market is flashed") l214 require(getCashPrior() == actualMintAmount, "CNote::mintFresh: Error in doTransferIn, CNote reserves >= mint Amount") l264 require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero") l310 require(getCashPrior() == 0, "CNote::redeemFresh, LendingMarket has > 0 Cash before Accountant Supplies") l330 require(getCashPrior() == redeemAmount, "CNote::redeemFresh: Accountant has supplied incorrect Amount")

stableswap/BaseV1-periphery.sol

l86 require(tokenA != tokenB, 'BaseV1Router: IDENTICAL_ADDRESSES') l104 require(amountA > 0, 'BaseV1Router: INSUFFICIENT_AMOUNT') l105 require(reserveA > 0 && reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY') l223 require(amountBOptimal >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT') l228 require(amountAOptimal >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT') l295 require(amountA >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT'); l296 require(amountB >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT') l387 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l402 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l415 require(routes[0].from == address(wcanto), 'BaseV1Router: INVALID_PATH') l417 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l430 require(amounts[amounts.length - 1] >= amountOutMin, 'BaseV1Router: INSUFFICIENT_OUTPUT_AMOUNT') l452 require(success, 'TransferHelper: ETH_TRANSFER_FAILED')

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

Tautologies

PROBLEM

Tautologies should be avoided as they waste gas.

PROOF OF CONCEPT

Instances include:

lending-market/NoteInterest.sol

l97 uint newRatePerYear = ir >= 0 ? ir : 0

ir is a uint256, hence always >= 0.

TOOLS USED

Manual Analysis

MITIGATION

Replace

uint newRatePerYear = ir >= 0 ? ir : 0;

with

uint newRatePerYear = ir;

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As uint8 and bool type variables are of size 1 byte, there's a slot here that can get saved by moving an address closer to a bool and or a uint8

PROOF OF CONCEPT

Instances include:

stableswap/BaseV1-core.sol

uint8 public constant decimals = 18; // Used to denote stable or volatile pair, not immutable since construction happens in the initialize method for CREATE2 deterministic addresses bool public immutable stable; uint public totalSupply = 0; mapping(address => mapping (address => uint)) public allowance; mapping(address => uint) public balanceOf; bytes32 internal DOMAIN_SEPARATOR; // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); bytes32 internal constant PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; mapping(address => uint) public nonces; uint internal constant MINIMUM_LIQUIDITY = 10**3; address public immutable token0; address public immutable token1; address immutable factory;

TOOLS USED

Manual Analysis

MITIGATION

Place factory before decimals to save one storage slot

+address immutable factory; uint8 public constant decimals = 18; // Used to denote stable or volatile pair, not immutable since construction happens in the initialize method for CREATE2 deterministic addresses bool public immutable stable; uint public totalSupply = 0; mapping(address => mapping (address => uint)) public allowance; mapping(address => uint) public balanceOf; bytes32 internal DOMAIN_SEPARATOR; // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); bytes32 internal constant PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; mapping(address => uint) public nonces; uint internal constant MINIMUM_LIQUIDITY = 10**3; address public immutable token0; address public immutable token1;

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

lending-market/WETH.sol

l30 _balanceOf[msg.sender] -= wad //cannot underflow because of check line 29 l73 _allowance[src][msg.sender] -= wad //cannot underflow because of check line 72 l76 _balanceOf[src] -= wad //cannot underflow because of check line 69

lending-market/GovernorBravoDelegate.sol

l68 i++ l90 i++

lending-market/Comptroller.sol

l126 i++ l206 i++ l735 i++ l959 i++ l1005 i++ l1106 i++ l1347 i++ l1353 j++ l1359 j++ l1364 j++ l1413 ++i

stableswap/BaseV1-core.sol

l156 uint timeElapsed = blockTimestamp - blockTimestampLast //as per comment, underflow desired l183 uint timeElapsed = blockTimestamp - _blockTimestampLast //as per comment, underflow desired l207 i++ l337 i++

stableswap/BaseV1-periphery.sol

l136 i++ l276 msg.value - amountCANTO //cannot underflow because of check line 276 l362 i++

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary functions

IMPACT

As of Solidity 0.8.0, underflow and overflow checks are default in mathematical operations. Using functions to perform these checks is redundant and wastes gas.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

180 function add256 186 function sub256

lending-market/NoteInterest.sol

using SafeMath for uint

TOOLS USED

Manual Analysis

MITIGATION

Remove these functions

Unnecessary computation

IMPACT

There are several instances where a local variable is used but is not necessary. For instance, when emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage.

PROOF OF CONCEPT

Instances include:

lending-market/GovernorBravoDelegate.sol

scope: _initiate()

l134:proposalCount = GovernorAlpha(governorAlpha).proposalCount()

scope: _setPendingAdmin()

address oldPendingAdmin = pendingAdmin; // Store pendingAdmin with value newPendingAdmin pendingAdmin = newPendingAdmin; // Emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin) emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);

scope: _acceptAdmin()

address oldAdmin = admin; address oldPendingAdmin = pendingAdmin; // Store admin with value pendingAdmin admin = pendingAdmin; // Clear the pending value pendingAdmin = address(0); emit NewAdmin(oldAdmin, admin); emit NewPendingAdmin(oldPendingAdmin, pendingAdmin)

lending-market/Comptroller.sol

scope: _setPriceOracle()

l833: PriceOracle oldOracle = oracle; // Set comptroller's oracle to newOracle oracle = newOracle; // Emit NewPriceOracle(oldOracle, newOracle) emit NewPriceOracle(oldOracle, newOracle)

scope: _setCloseFactor()

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

scope: _setLiquidationIncentive()

l916 uint oldLiquidationIncentiveMantissa = liquidationIncentiveMantissa; // Set liquidation incentive to new incentive liquidationIncentiveMantissa = newLiquidationIncentiveMantissa; // Emit event with old incentive, new incentive emit NewLiquidationIncentive(oldLiquidationIncentiveMantissa, newLiquidationIncentiveMantissa)

scope: _setBorrowCapGuardian()

l1019 address oldBorrowCapGuardian = borrowCapGuardian; // Store borrowCapGuardian with value newBorrowCapGuardian borrowCapGuardian = newBorrowCapGuardian; // Emit NewBorrowCapGuardian(OldBorrowCapGuardian, NewBorrowCapGuardian) emit NewBorrowCapGuardian(oldBorrowCapGuardian, newBorrowCapGuardian)

scope: _setPauseGuardian()

l1038 address oldPauseGuardian = pauseGuardian; // Store pauseGuardian with value newPauseGuardian pauseGuardian = newPauseGuardian; // Emit NewPauseGuardian(OldPauseGuardian, NewPauseGuardian) emit NewPauseGuardian(oldPauseGuardian, pauseGuardian);

lending-market/TreasuryDelegate.sol

scope: queryCantoBalance()

l26 uint treasuryCantoBalance = address(this).balance

scope: querynoteBalance()

l35 uint treasuryNoteBalance = note.balanceOf(address(this))

lending-market/TreasuryDelegator.sol

scope: _setImplementation

l34 address oldImplementation = implementation; implementation = implementation_; emit NewImplementation(oldImplementation, implementation_)

lending-market/NoteInterest.sol

scope: _setBaseRatePerYear

l142 uint oldBaseRatePerYear = baseRatePerYear; baseRatePerYear = newBaseRateMantissa; emit NewBaseRate(oldBaseRatePerYear, baseRatePerYear)

scope: _setAdjusterCoefficient

l155 uint oldAdjusterCoefficient = adjusterCoefficient; adjusterCoefficient = newAdjusterCoefficient; emit NewAdjusterCoefficient(oldAdjusterCoefficient, adjusterCoefficient)

scope: _setUpdateFrequency

l168 uint oldUpdateFrequency = updateFrequency; updateFrequency = newUpdateFrequency; emit NewUpdateFrequency(oldUpdateFrequency, updateFrequency)

manifest/Proposal-Store.sol

l42 Proposal memory prop = Proposal(propId, title, desc, targets, values, signatures, calldatas); l48 Proposal memory newProp = Proposal(propId, title, desc, targets, values, signatures, calldatas);

TOOLS USED

Manual Analysis

MITIGATION

Remove the unnecessary local variables:

  • write to the storage variable directly instead of using an intermediate local variable
  • Emit the event before writing to the storage variable to avoid the one local variable computation

e.g:

address oldPendingAdmin = pendingAdmin; pendingAdmin = newPendingAdmin; emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);

with

emit NewPendingAdmin(pendingAdmin, newPendingAdmin); pendingAdmin = newPendingAdmin;

#0 - GalloDaSballo

2022-08-04T00:30:15Z

About 5.5k (4.2k immutables) rest is less than 1k

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter