Platform: Code4rena
Start Date: 18/10/2022
Pot Size: $75,000 USDC
Total HM: 27
Participants: 144
Period: 7 days
Judge: gzeon
Total Solo HM: 13
Id: 170
League: ETH
Rank: 66/144
Findings: 2
Award: $55.67
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Rolezn
Also found by: 0x1f8b, 0x52, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xhunter, 0xzh, 8olidity, Amithuddar, Aymen0909, B2, Bnke0x0, Chom, Deivitto, Diana, Diraco, Dravee, Franfran, JC, Jeiwan, Josiah, JrNet, Jujic, KingNFT, KoKo, Lambda, Margaret, Migue, Ocean_Sky, PaludoX0, Picodes, Rahoz, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Shinchan, Tagir2003, Trust, Waze, Yiko, __141345__, a12jmx, adriro, ajtra, arcoun, aysha, ballx, bin2chen, bobirichman, brgltd, bulej93, catchup, catwhiskeys, caventa, cccz, cdahlheimer, ch0bu, chaduke, chrisdior4, cloudjunky, cryptostellar5, cryptphi, csanuragjain, cylzxje, d3e4, delfin454000, djxploit, durianSausage, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, hansfriese, i_got_hacked, ignacio, imare, karanctf, kv, leosathya, louhk, lukris02, lyncurion, m_Rassska, malinariy, martin, mcwildy, mics, minhtrng, nicobevi, oyc_109, pashov, peanuts, pedr02b2, peiw, rbserver, ret2basic, rotcivegaf, rvierdiiev, ryshaw, sakman, sakshamguruji, saneryee, securerodd, seyni, sikorico, svskaushik, teawaterwire, tnevler, w0Lfrum
55.6726 USDC - $55.67
Unhandled return values of transfer and transferFrom: ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures.
Recommendation: Check the return value and revert on 0/false or use OpenZeppelin’s SafeERC20 wrapper functions
Medium severity finding from Consensys Diligence Audit of Aave Protocol V2 https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom
Random task execution In a scenario where a user takes a flash loan, _parseFLAndExecute() gives the flash loan wrapper contract (FLAaveV2, FLDyDx) the permission to execute functions on behalf of the user’s DSProxy. This execution permission is revoked only after the entire recipe execution is finished, which means that in case that any of the external calls along the recipe execution is malicious, it might call executeAction() back, i.e. Reentrancy Attack, and inject any task it wishes (e.g. take user’s funds out, drain approved tokens, etc)
Recommendation: A reentrancy guard (mutex) should be used to prevent such attack
Critical severity finding from Consensys Diligence Audit of Defi Saver: https://consensys.net/diligence/audits/2021/03/defi-saver/#random-task-execution
Critical address change Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. https://github.com/OpenZeppelin/openzeppelin-contracts/issues/1488 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2369
Missing events Events for critical state changes (e.g. owner and other critical parameters) should be emitted for tracking this off-chain. https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-access-control
Unindexed event parameters Parameters of certain events are expected to be indexed (e.g. ERC20 Transfer/Approval events) so that they’re included in the block’s bloom filter for faster access. Failure to do so might confuse off-chain tooling looking for such indexed events. https://github.com/crytic/slither/wiki/Detector-Documentation#unindexed-erc20-event-oarameters
Avoid transfer()/send() as reentrancy mitigations: Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes may break deployed contracts. Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ https://swcregistry.io/docs/SWC-134
ERC20 approve() race condition Use safeIncreaseAllowance() and safeDecreaseAllowance() from OpenZeppelin’s SafeERC20 implementation to prevent race conditions from manipulating the allowance amounts. https://swcregistry.io/docs/SWC-114
fallback vs receive() Check that all precautions and subtleties of fallback/receive functions related to visibility, state mutability and Ether transfers have been considered. https://docs.soliditylang.org/en/latest/contracts.html#fallback-function https://docs.soliditylang.org/en/latest/contracts.html#receive-ether-function
Tautology or contradiction Tautologies (always true) or contradictions (always false) indicate potential flawed logic or redundant checks. e.g. x >= 0 which is always true if x is uint. https://github.com/crytic/slither/wiki/Detector-Documentation#tautology-or-contradiction
uint256(1) == 1 ? true
Dangerous strict equalities: Use of strict equalities with tokens/Ether can accidentally/maliciously cause unexpected behavior. Consider using >= or <= instead of == for such variables depending on the contract logic. https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities
Dangerous usage of tx.origin: Use of tx.origin for authorization may be abused by a MITM malicious contract forwarding calls from the legitimate user who interacts with it. Use msg.sender instead.
🌟 Selected for report: oyc_109
Also found by: 0x040, 0x1f8b, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xsam, 0xzh, 2997ms, Amithuddar, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, Franfran, JC, JrNet, Jujic, KingNFT, KoKo, Mathieu, Metatron, Mukund, Olivierdem, PaludoX0, Pheonix, Picodes, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Saintcode_, Satyam_Sharma, Shinchan, Tagir2003, Tomio, Waze, Yiko, __141345__, adriro, ajtra, aysha, ballx, beardofginger, bobirichman, brgltd, bulej93, catchup, catwhiskeys, cdahlheimer, ch0bu, chaduke, chrisdior4, cryptostellar5, cylzxje, d3e4, delfin454000, dharma09, djxploit, durianSausage, emrekocak, erictee, exolorkistis, fatherOfBlocks, gianganhnguyen, gogo, halden, hxzy, i_got_hacked, iepathos, karanctf, leosathya, lucacez, lukris02, lyncurion, m_Rassska, martin, mcwildy, mics, nicobevi, peanuts, peiw, rbserver, ret2basic, rotcivegaf, ryshaw, sakman, sakshamguruji, saneryee, sikorico, skyle, svskaushik, tnevler, vv7, w0Lfrum, zishansami
0 USDC - $0.00
Use Custom Errors Instead of Revert Strings to Save Gas Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Splitting require() statements that use && saves gas See this issue: code-423n4/2022-01-xdefi-findings#128 which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper
= costs less gas than > The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographBridge.sol#L218 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L309 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L350 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L363 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L398 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L415 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L519 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L1126 ==========================================================
Using > 0 costs more gas than != 0 when used on a uint in a require() statement This change saves 6 gas https://aws1.discourse-cdn.com/business6/uploads/zeppelin/original/2X/3/363a367d6d68851f27d2679d10706cd16d788b96.png per instance. The optimization works until solidity version 0.8.13 https://gist.github.com/IllIllI000/bf2c3120f24a69e489f12b3213c06c94 where there is a regression in gas costs.
Prefix Increments Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops. Change variable++ to ++variable. https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L100
Events Indexing Events should use the maximum amount of indexed fields: up to three parameters. This makes it easier to filter for specific values in front-ends.
Proof of Concept
Recommended Mitigation Steps Add indexed fields to these events so that they have the maximum number of indexed fields possible.
Empty Blocks Should Be Removed Or Emit Something The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}) https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/abstract/ERC20H.sol#L212 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/abstract/ERC721H.sol#L212 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/Holographer.sol#L223 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC20.sol#L251 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC721.sol#L962