Platform: Code4rena
Start Date: 23/05/2022
Pot Size: $50,000 USDC
Total HM: 44
Participants: 99
Period: 5 days
Judge: hickuphh3
Total Solo HM: 11
Id: 129
League: ETH
Rank: 36/99
Findings: 6
Award: $224.38
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: xiaoming90
Also found by: GimelSec, IllIllI, MaratCerby, PP1004, WatchPug, berndartmueller, blockdev, ilan
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L202-L206 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L274-L278 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L366 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L157-L161 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L172-L176
Every time transferFrom or transfer function in ERC20 standard is called there is a possibility that underlying smart contract did not transfer the exact amount entered. It is required to find out contract balance increase/decrease after the transfer to allow fee-on-transfer tokens or forbid non-standard tokens. This pattern also prevents from re-entrancy attack vector.
POC (re-entrancy for fee-on-transfer tokens):
There are several possible scenarios to prevent that.
Recommended example code:
enum FeeOnTransferStatuses{ NOT_INITIALIZED, HAS_FEE_ON_TRANSFER, DOES_NOT_HAVE_FEE_ON_TRANSFER } mapping(IERC20 => FeeOnTransferStatuses) doesThisContractHaveFeeOnTransfer; error FeeOnTransferTokensAreForbidden(); ... function deposit(IERC20 token, address from, uint256 amount) public { // reverting for fee-on-transfer tokens if (doesThisContractHaveFeeOnTransfer[token] == FeeOnTransferStatuses.HAS_FEE_ON_TRANSFER) { revert FeeOnTransferTokensAreForbidden(); } // NOT_INITIALIZED is the default value == 0 if (doesThisContractHaveFeeOnTransfer[token] == FeeOnTransferStatuses.NOT_INITIALIZED) { uint256 balanceBefore = token.balanceOf(address(this)); // remembering asset balance before the transfer token.safeTransferFrom(from, address(this), amount); uint256 balanceAfter = token.balanceOf(address(this)); // making sure exactly amount was transferred if (balanceAfter != balanceBefore + amount) { revert FeeOnTransferTokensAreForbidden(); } // IMPORTANT! if you allow fee-on-transfer tokens make sure to update amount with the actual balance increase/decrease // or make sure balanceAfter - balanceBefore == amount using require // amount = balanceAfter - balanceBefore; // commented because we skip fee-on-transfer tokens above doesThisContractHaveFeeOnTransfer[token] = FeeOnTransferStatuses.DOES_NOT_HAVE_FEE_ON_TRANSFER; // making sure to not enter this if clause anymore for given token } else { // token does not have fee-on-transfer here token.safeTransferFrom(from, address(this), amount); } // no re-entrancy vector attack below here // amount is set to exactly how much contract balance was increased registerDeposit(from, amount); }
#0 - bghughes
2022-06-03T20:42:59Z
Duplicate of #316
#1 - HickupHH3
2022-06-15T13:02:25Z
Duplicate of #112 because it is about FoT
#2 - HickupHH3
2022-06-16T08:06:22Z
A good auditor provides meaningful context and explanation to the issue. The consensus is that we shouldn't be rewarding wardens who do not put in the effort to help the sponsors understand issues raised, compared to other submissions that actually do.
Hence, I would've marked this as QA because the POC and example code is generic and isn't customized. However, in the spirit of fairness to the numerous duplicates regarding another issue about checking ERC20 return values, I will leave things the way they are this time around.
🌟 Selected for report: kenzo
Also found by: 0x1f8b, 0xsomeone, Dravee, IllIllI, MaratCerby, berndartmueller, cryptphi, xiaoming90
https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L157 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L165 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L180 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L214 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L256 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L689 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L744
a. ERC20 standard allows approve function of some contracts to return bool or return nothing Using safeApprove of SafeERC20.sol is recommended instead.
b. Approval must be set to zero and after that increased to the amount you need. Some of the tokens such as USDT require that. Please read more information here: https://www.adrianhetman.com/unboxing-erc20-approve-issues/
#0 - bghughes
2022-06-03T20:43:41Z
Duplicate of #316
#1 - HickupHH3
2022-06-23T15:19:53Z
Weak duplicate of #100
🌟 Selected for report: WatchPug
Also found by: Chom, Dravee, Hawkeye, MaratCerby, Ruhum, csanuragjain, fatherOfBlocks, minhquanym
42.6857 USDC - $42.69
Judge has assessed an item in Issue #27 as Medium risk. The relevant finding follows:
#0 - HickupHH3
2022-06-27T14:00:49Z
isClosed always returns false, consider removing it because it makes no sense. dup of #148
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0x4non, 0xDjango, 0xKitsune, 0xNazgul, 0xf15ers, ACai, AlleyCat, Bahurum, BouSalman, CertoraInc, Chom, Dravee, ElKu, FSchmoede, Funen, GimelSec, Hawkeye, JC, JMukesh, Kaiziron, MaratCerby, Metatron, PP1004, Picodes, Ruhum, SmartSek, StErMi, TerrierLover, UVvirus, UnusualTurtle, WatchPug, Waze, _Adam, asutorufos, berndartmueller, blackscale, blockdev, broccolirob, c3phas, catchup, cryptphi, csanuragjain, defsec, delfin454000, dipp, eccentricexit, ellahi, fatherOfBlocks, gzeon, hansfriese, horsefacts, hubble, ilan, joestakey, kebabsec, minhquanym, oyc_109, parashar, pauliax, rotcivegaf, sach1r0, sashik_eth, shenwilly, simon135, sorrynotsorry, sseefried, throttle, unforgiven, xiaoming90
52.8093 USDC - $52.81
[1] isClosed always returns false, consider removing it because it makes no sense.
Affected code:
[2] Using if (val) is easier to read rather than if (val == true). Using if (!val) is easier to read rather than if (val == false). Consider updating all occurrences.
Affected code:
[3] By default, function types and state variables/constants are internal, so the internal keyword can be omitted.
Affected code:
[4] Using x != 0 uses 6 less gas than x > 0. Consider changing all "greater than zero" comparisons to "not equal to zero".
Affected code:
[5] Magic number, consider using named constant instead.
Affected code:
[6] Replacing "_msgSender()" to "msg.sender" will save gas.
Affected code:
[7] Consider using "_" separate digit capacity i.e "100000" could be replaced to "100_000". This increases code readability.
Affected code:
[8] Consider using IERC20 type instead of address. Or IERC20[] type instead of address[].
Affected code:
[9] Uint8-256 / Int8-256 is assigned to zero by default, additional reassignment to zero is unnecessary.
Affected code:
[10] The value like "10256 - 1" (or "10128 - 1") could be replaced to "type(uint256).max" (or "type(uint128).max") accordingly.
Affected code:
[11] Events are usually named in present tense such as TokenTransfer. Consider renaming this event to OfferDelete.
Affected code:
[12] Better way to initialize structs is
uint256 x1; uint256 x2; } Something memory smth = Something({x1: 123, x2: 321});``` Affected code: 1. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L406-L412 ## Proof of Concept ## Tools Used ## Recommended Mitigation Steps --- ## Impact [13] IF nesting could be reduced here by having early return/continue. Affected code: 1. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L849-L863 2. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L888-L904 3. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L1009-L1033 4. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L494-L501 5. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L634-L652 6. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L230-L267 7. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L371-L378 ## Proof of Concept ## Tools Used ## Recommended Mitigation Steps ---
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, Chom, DavidGialdi, Dravee, ElKu, FSchmoede, Fitraldys, Funen, GimelSec, JC, Kaiziron, MaratCerby, Metatron, MiloTruck, Picodes, Randyyy, RoiEvenHaim, SmartSek, Tomio, UnusualTurtle, WatchPug, Waze, _Adam, antonttc, asutorufos, berndartmueller, blackscale, blockdev, c3phas, catchup, csanuragjain, defsec, delfin454000, ellahi, fatherOfBlocks, gzeon, hansfriese, ilan, joestakey, minhquanym, oyc_109, pauliax, pedroais, reassor, rfa, rotcivegaf, sach1r0, samruna, sashik_eth, simon135, z3s
43.4023 USDC - $43.40
[1] Token decimals is a constant parameter that never changes. Consider caching it.
Affected code:
[2] Consider using optimized for-loop and apply the following optimizations:
Affected code:
[3] Splitting && conditions into several require statements saves gas.
Affected code:
[4] Using ++x uses 5 less gas than x++ (same applies to --x). Consider changing all "x++" operators to "++x" (same applies to --x).
Affected code:
[5] The power of 10 numbers such as "10**18" could be rendered as "1e18".
Affected code:
[6] You can upgrade to 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code:
[7] uint32 is enough to store timestamp.
Affected code:
[8] It is better to use uint256 for locked variable and set it 1 for unlocked state and to 2 for locked stake or use enums for that. That way you will reduce runtime gas usage for locking/unlocking.
Affected code:
[9] Using storage keyword here can save up-to 700 gas here.
Affected code:
[10] Using struct array will allow you to avoid length checks.
Affected code: