Canto contest - Funen's results

1. Title : Constant can be changed for good

Since min, hour, days, month or year can be used than using a number.

1.) File : BaseV1-core.sol Line.70

 uint constant periodSize = 1800; //every 30 minutes

changed into 1800 into 30 minutes

2. Title : Unused Var state

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L62

        newProposal.canceled = false;

The newProposal.canceled was unused in the contract since it no call use it. so it can be deleted instead

3. Title : SPDX License was not set in WETH

SPDX license identifier not provided in source file WETH.sol. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code.

Recommended Mitigation

Use SPDX-License

4. TItle : No Return was used Function Allowance()

Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable.

The function is declared as returning uint256, but the code doesn't return anything.

Use implementation below to used return :

 return _allowances[owner][spender];

As all variables are automatically assigned a "zero value" (including the return variables), not using "return" is not unsafe but it might indicate a programming error.

Especially if you name the return variables, code that does not use an explicit "return" is sometimes easier to read and check.

5. Lack of an event for WETH.sol

Since it was important role for Deposit and Withdrawal be having an event so it can be used inside the contract for better use, since it was Standard Wrapped Ether contract.

##POC

Event is an inheritable member of a contract. An event is emitted, it stores the arguments passed in transaction logs. These logs are stored on blockchain and are accessible using address of the contract till the contract is present on the blockchain. An event generated is not accessible from within contracts, not even the one which have created and emitted them.

https://www.tutorialspoint.com/solidity/solidity_events.htm

1.) File : WETH.sol Line.24

         /* emit Deposit(msg.sender, msg.value); */ 

2.) File : WETH.sol Line.32

        /* emit Withdrawal(msg.sender, wad); */

3.) File : WETH.sol Lines.106-107

    /* event  Deposit(address indexed dst, uint wad); */
    /* event  Withdrawal(address indexed src, uint wad); */

6. Title : Uncleared arity mean in queue()

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L45

in this require state, short reason string was unclear since no arity mean. is it typo or it mean clarity or something, who knows. Since it was confusing. it can be changed or it can be deleted instead.

7. Title : using pragma abicoder V2

Since AccountantDelegator.sol was using pragma experimental ABIEncoderV2 and used pragma ^0.7.6, it because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2 instead since Solidity 0.7.4.

POC

https://docs.soliditylang.org/en/latest/080-breaking-changes.html https://docs.soliditylang.org/en/latest/layout-of-source-files.html?highlight=experimental#abiencoderv2

Tools

Manual Review

Recommended Mitigation

Add pragma abicoder v1; if you want to stay with the old ABI coder. Optionally remove pragma experimental ABIEncoderV2 or pragma abicoder v2 since it is redundant.

8. Title : Change reason string for better readibility

1.) File : NoteInterest.sol Line.141

        require(msg.sender == admin, "only the admin may set the base rate");

change to :

only admin may set base rate

2.) File : NoteInterest.sol Line.154

        require(msg.sender == admin, "only the admin may set the adjuster coefficient");

change to :

only admin may set adjuster coefficient

3.) File : NoteInterest.sol Line.167

        require(msg.sender == admin, "only the admin may set the update frequency");

change to :

only admin may set update frequency

1. Title : Using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

/contracts/BaseV1-core.sol#L207                    for (uint i = 0; i < _prices.length; i++) {
/contracts/BaseV1-core.sol#L207                    for (uint i = 0; i < 255; i++) 
/contracts/BaseV1-periphery.sol#L136            for (uint i = 0; i < routes.length; i++) {
/contracts/BaseV1-periphery.sol#L362            for (uint i = 0; i < routes.length; i++) {
/contracts/Governance/GovernorBravoDelegate.sol#L68        for (uint i = 0; i < newProposal.targets.length; i++) {
/contracts/Governance/GovernorBravoDelegate.sol#L90        for (uint i = 0; i < proposal.targets.length; i++) {
/contracts/Comptroller.sol#L126                       for (uint i = 0; i < len; i++) {
/contracts/Comptroller.sol#L206                      for (uint i = 0; i < len; i++) {
/contracts/Comptroller.sol#L735                      for (uint i = 0; i < assets.length; i++) {
/contracts/Comptroller.sol#L959                      for (uint i = 0; i < allMarkets.length; i ++) {
/contracts/Comptroller.sol#L1005                     for(uint i = 0; i < numMarkets; i++) {
/contracts/Comptroller.sol#L1347                     for (uint i = 0; i < cTokens.length; i++) {
/contracts/Comptroller.sol#L1353                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1359                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1364                     for (uint j = 0; j < holders.length; j++) {

2. Title : change uint i = 0 into uint i for saving more gas

using this implementation can saving more gas for each loops.

Tool Used

Manual Review

Recommended Mitigation

Change it

Occurances

/contracts/BaseV1-core.sol#L207                    for (uint i = 0; i < _prices.length; i++) {
/contracts/BaseV1-core.sol#L207                    for (uint i = 0; i < 255; i++) 
/contracts/BaseV1-periphery.sol#L136            for (uint i = 0; i < routes.length; i++) {
/contracts/BaseV1-periphery.sol#L362            for (uint i = 0; i < routes.length; i++) {
/contracts/Governance/GovernorBravoDelegate.sol#L68        for (uint i = 0; i < newProposal.targets.length; i++) {
/contracts/Governance/GovernorBravoDelegate.sol#L90        for (uint i = 0; i < proposal.targets.length; i++) {
/contracts/Comptroller.sol#L126                       for (uint i = 0; i < len; i++) {
/contracts/Comptroller.sol#L206                      for (uint i = 0; i < len; i++) {
/contracts/Comptroller.sol#L735                      for (uint i = 0; i < assets.length; i++) {
/contracts/Comptroller.sol#L959                      for (uint i = 0; i < allMarkets.length; i ++) {
/contracts/Comptroller.sol#L1005                     for(uint i = 0; i < numMarkets; i++) {
/contracts/Comptroller.sol#L1347                     for (uint i = 0; i < cTokens.length; i++) {
/contracts/Comptroller.sol#L1353                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1359                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1364                     for (uint j = 0; j < holders.length; j++) {

3. Title : Title : Caching array length can saving more gas

This implementation can be saving more gas, since if caching the array length is more gas efficient. just because access to a local variable in solidity is more efficient.

Tool Used

Manual Review

Occurances

/contracts/Governance/GovernorBravoDelegate.sol#L68        for (uint i = 0; i < newProposal.targets.length; i++) {
/contracts/Governance/GovernorBravoDelegate.sol#L90        for (uint i = 0; i < proposal.targets.length; i++) {
/contracts/Comptroller.sol#L1353                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1359                     for (uint j = 0; j < holders.length; j++) {
/contracts/Comptroller.sol#L1364                     for (uint j = 0; j < holders.length; j++) {

4 Title : Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

Tool Used

Manual Review

Occurances

/contracts/WETH.sol#L29 "sender balance insufficient for withdrawal"
/contracts/WETH.sol#L96  "ERC20: approve from the zero address"
/contracts/WETH.sol#L97 "ERC20: approve to the zero address"
/contracts/Comptroller.sol#L178 "exitMarket: getAccountSnapshot failed"
/contracts/Comptroller.sol#L491 "Can not repay more than the total borrow"
/contracts/Comptroller.sol#L998 "only admin or borrow cap guardian can set borrow caps"
/contracts/Comptroller.sol#L1016 "only admin can set borrow cap guardian"
/contracts/Comptroller.sol#L1051 "cannot pause a market that is not listed"
/contracts/Comptroller.sol#L1052  "only pause guardian and admin can pause"
/contracts/Comptroller.sol#L1061 "cannot pause a market that is not listed"
/contracts/Comptroller.sol#L1062  "only pause guardian and admin can pause"
/contracts/Comptroller.sol#L1071 "only pause guardian and admin can pause"
/contracts/Comptroller.sol#L1080 "only pause guardian and admin can pause"
/contracts/Comptroller.sol#L1089 "only unitroller admin can change brains"
/contracts/Comptroller.sol#L1095 "Only admin can call this function"
/contracts/Comptroller.sol#L1096 "Already executed this one-off function"
/contracts/Governance/GovernorBravoDelegate.sol#L25 "GovernorBravo::initialize: can only initialize once"
/contracts/Governance/GovernorBravoDelegate.sol#L26 "GovernorBravo::initialize: admin only"
/contracts/Governance/GovernorBravoDelegate.sol#L27 "GovernorBravo::initialize: invalid timelock address"
/contracts/Governance/GovernorBravoDelegate.sol#L45 "GovernorBravo::propose: proposal function information arity mismatch"
/contracts/Governance/GovernorBravoDelegate.sol#L46 "GovernorBravo::propose: must provide actions"
/contracts/Governance/GovernorBravoDelegate.sol#L47 "GovernorBravo::propose: too many actions"
/contracts/Governance/GovernorBravoDelegate.sol#L78 "GovernorBravo::queueOrRevertInternal: identical proposal action already queued at eta"
/contracts/Governance/GovernorBravoDelegate.sol#L87 "GovernorBravo::execute: proposal can only be executed if it is queued"
/contracts/Governance/GovernorBravoDelegate.sol#L133 "GovernorBravo::_initiate: admin only"
/contracts/Governance/GovernorBravoDelegate.sol#L134 "GovernorBravo::_initiate: can only initiate once"
/contracts/Governance/GovernorBravoDelegate.sol#L146 "GovernorBravo:_setPendingAdmin: admin only"
/contracts/Governance/GovernorBravoDelegate.sol#L164 "GovernorBravo:_acceptAdmin: pending admin only"
/contracts/Accountant/AccountantDelegator.sol#L43 "AccountantDelegator::_setImplementation: admin only"
/contracts/Accountant/AccountantDelegator.sol#L44 "AccountantDelegator::_setImplementation: invalid implementation address"
/contracts/Accountant/AccountantDelegator.sol#L124 "AccountantDelegator:fallback: cannot send value to fallback"
/contracts/Accountant/AccountantDelegate.sol#L17 "AccountantDelegate::initialize: only admin can call this function"
/contracts/Accountant/AccountantDelegate.sol#L18 "AccountantDelegate::initialize: note Address invalid"
/contracts/Accountant/AccountantDelegate.sol#L29 "AccountantDelegate::initiatlize: Accountant has not received payment"
/contracts/Accountant/AccountantDelegate.sol#L48 "AccountantDelegate::supplyMarket: Only the CNote contract can supply market"
/contracts/Accountant/AccountantDelegate.sol#L60 "AccountantDelegate::redeemMarket: Only the CNote contract can redeem market"
/contracts/Accountant/AccountantDelegate.sol#L83 "Note Loaned to LendingMarket must increase in value"
/contracts/Treasury/TreasuryDelegator.sol#L31 "GovernorBravoDelegator::_setImplementation: admin only"
/contracts/Treasury/TreasuryDelegator.sol#L32 "GovernorBravoDelegator::_setImplementation: invalid implementation address"
/contracts/Treasury/TreasuryDelegate.sol#L47 "Treasury::sendFund can only be called by admin"

5. CUSTOM ERRORS

Custom errors can be used from Solidity 0.8.4 are cheaper than revert strings. Its cheaper deployment cost and runtime cost when the revert condition is met.

POC

https://blog.soliditylang.org/2021/04/21/custom-errors/

Occurances

/contracts/WETH.sol#L29
/contracts/WETH.sol#L96  
/contracts/WETH.sol#L97
/contracts/Comptroller.sol#L178
/contracts/Comptroller.sol#L491 
/contracts/Comptroller.sol#L998 
/contracts/Comptroller.sol#L1016 
/contracts/Comptroller.sol#L1051
/contracts/Comptroller.sol#L1052  
/contracts/Comptroller.sol#L1061 
/contracts/Comptroller.sol#L1062  
/contracts/Comptroller.sol#L1071 
/contracts/Comptroller.sol#L1080 
/contracts/Comptroller.sol#L1089 
/contracts/Comptroller.sol#L1095 
/contracts/Comptroller.sol#L1096 
/contracts/Governance/GovernorBravoDelegate.sol#L25 
/contracts/Governance/GovernorBravoDelegate.sol#L26 
/contracts/Governance/GovernorBravoDelegate.sol#L27
/contracts/Governance/GovernorBravoDelegate.sol#L45 
/contracts/Governance/GovernorBravoDelegate.sol#L46 
/contracts/Governance/GovernorBravoDelegate.sol#L47 
/contracts/Governance/GovernorBravoDelegate.sol#L78 
/contracts/Governance/GovernorBravoDelegate.sol#L87 
/contracts/Governance/GovernorBravoDelegate.sol#L133 
/contracts/Governance/GovernorBravoDelegate.sol#L134 
/contracts/Governance/GovernorBravoDelegate.sol#L146 
/contracts/Governance/GovernorBravoDelegate.sol#L164 
/contracts/Accountant/AccountantDelegator.sol#L43
/contracts/Accountant/AccountantDelegator.sol#L44 
/contracts/Accountant/AccountantDelegator.sol#L124 
/contracts/Accountant/AccountantDelegate.sol#L17 
/contracts/Accountant/AccountantDelegate.sol#L29
/contracts/Accountant/AccountantDelegate.sol#L48
/contracts/Accountant/AccountantDelegate.sol#L60 
/contracts/Accountant/AccountantDelegate.sol#L83 
/contracts/Treasury/TreasuryDelegator.sol#L31 
/contracts/Treasury/TreasuryDelegator.sol#L32 
/contracts/Treasury/TreasuryDelegate.sol#L47