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
Rank: 53/59
Findings: 1
Award: $105.36
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: _Adam
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xmint, Chom, Dravee, Fitraldys, Funen, JC, Limbooo, MadWookie, Picodes, Ruhum, TerrierLover, TomJ, Tomio, Waze, ak1, c3phas, catchup, defsec, fatherOfBlocks, gzeon, hake, hansfriese, joestakey, k, oyc_109, rfa, robee, sach1r0, saian, simon135, ynnad
41.2642 USDC - $41.26
396.9199 CANTO - $64.10
Title: Reduce the size of error messages (Long revert Strings)
Impact: Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Accountant/AccountantDelegator.sol#L43-L44 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Accountant/AccountantDelegate.sol#L17-L18 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol#L25-L27 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L137-L140
Recommended Mitigation Steps: Consider shortening the revert strings to fit in 32 bytes
Title: Custom errors from Solidity 0.8.4 are cheaper than revert strings
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
Custom errors are defined using the error statement reference: https://blog.soliditylang.org/2021/04/21/custom-errors/
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Accountant/AccountantDelegator.sol https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Accountant/AccountantDelegate.sol https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol
Recommended Mitigation Steps: Replace require statements with custom errors.
Title: >=
is cheaper than >
Impact:
Strict inequalities (>
) are more expensive than non-strict ones (>=
). This is due to some supplementary checks (ISZERO, 3 gas)
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L179
Recommended Mitigation Steps:
Consider using >=
instead of >
to avoid some opcodes
Title: Consider make constant as private to save gas
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L6-L15 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L110-L113 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol#L9-L18
Recommended Mitigation Steps:
I suggest changing the visibility from public
to internal
or private
Title: Using msg.sender
directly is more effective
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L129
Recommended Mitigation Steps:
In function transferFrom() using msg.sender
directly instead of caching it to spender
. delete L#129 and replace all spender
with msg.sender
Title: Comparison operators
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L168 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L287 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L292
Recommended Mitigation Steps:
Replace <=
with <
, and >=
with >
for gas opt
Title: Using !=
is more gas efficient
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L228 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L245
Recommended Mitigation Steps:
Change from >
to !=
require(proposalCount >= proposalId && proposalId != 0, "GovernorAlpha::state: invalid proposal id");
Title: Expression for constant
values such as a call to keccak256()
, should use immutable
rather than constant
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L39-L42 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L110-L113 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol#L15-L18
Recommended Mitigation Steps:
Change from constant
to immutable
reference: https://github.com/ethereum/solidity/issues/9232
Title: Gas optimization to dividing by 2
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L210
Recommended Mitigation Steps:
Replace / 2
with >> 1
Title: Using multiple require
instead of &&
can save gas
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L138 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L228 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol#L115
Recommended Mitigation Steps: Change to:
require(proposalCount >= proposalId, "GovernorAlpha::state: invalid proposal id"); require(proposalId > 0, "GovernorAlpha::state: invalid proposal id");
Title: Using unchecked and prefix increment is more effective for gas saving:
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L181 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L197 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L211
Recommended Mitigation Steps: Change to:
for (uint i = 0; i < proposal.targets.length;) { // ... unchecked { ++i; } }
Title: Using == false
cost more gas
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L266
Recommended Mitigation Steps: Change to:
require(!receipt.hasVoted, "GovernorAlpha::_castVote: voter already voted");
Title: 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 gas.
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/Comp.sol#L207 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorAlpha.sol#L181 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Governance/GovernorBravoDelegate.sol#L68
Recommended Mitigation Steps: Remove explicit initialization for default values.
Title: Caching length
for loop can save gas
Proof of Concept: https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L150 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Lens/CompoundLens.sol#L402-L404 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Lens/CompoundLens.sol#L474-L476
Recommended Mitigation Steps: Change to:
uint256 Length = proposalIds.length; GovProposal[] memory res = new GovProposal[](Length); for (uint i = 0; i < Length; i++) {
Title: Return statement should be unchecked
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CErc20.sol#L186
see @audit
tag:
return balanceAfter - balanceBefore; // underflow already checked above, just subtract. @audit gas: from the comment can be unchecked
Title: Using +=
can save gas
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CToken.sol#L439-L440
Recommended Mitigation Steps: Change to:
totalSupply += mintTokens; accountTokens[minter] += mintTokens;
17,
Title: Should use -=
instead
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CToken.sol#L532-L533
Recommended Mitigation Steps: Change to:
totalSupply -= redeemTokens; accountTokens[redeemer] -= redeemTokens;
Title: Boolean comparison
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L149
Recommended Mitigation Steps:
I suggest using if(marketToJoin.accountMembership[borrower])
instead of if(marketToJoin.accountMembership[borrower] == true)
if (marketToJoin.accountMembership[borrower]) {
Title: Unused parameter
Impact: Unused parameters clutter the code and slightly increase contract usage costs
Proof of Concept: https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L239-L241 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L262-L266 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L324-L326 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L399-L402
Recommended Mitigation Steps: Consider removing all unused parameters
#0 - GalloDaSballo
2022-08-04T00:45:17Z
Title: Expression for constant values such as a call to keccak256(), should use immutable rather than constant Not true -> https://twitter.com/GalloDaSballo/status/1543729080926871557
Rest is less than 500 gas