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: 15/99
Findings: 4
Award: $1,097.89
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: berndartmueller
Also found by: 0x1f8b, 0xDjango, 0xsomeone, ACai, Bahurum, BouSalman, CertoraInc, Deivitto, Dravee, GimelSec, IllIllI, JMukesh, Kaiziron, PP1004, Ruhum, SmartSek, VAD37, WatchPug, _Adam, aez121, antonttc, blockdev, broccolirob, camden, cccz, cryptphi, defsec, dipp, ellahi, fatherOfBlocks, gzeon, horsefacts, ilan, jayjonah8, joestakey, kenta, kenzo, minhquanym, oyc_109, pauliax, pedroais, peritoflores, sashik_eth, shenwilly, simon135, throttle, xiaoming90, z3s
0.1022 USDC - $0.10
https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L297-L300 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L304-L311 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L361 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L416
As the RubiconMarket contract is meant to be agnostic of the assets traded support for a wide array of the top cryptocurrencies is expected. One of the top ones is USDT (Tether), which the codebase is incompatible with due to the unique way the USDT transfers are non-compliant with the EIP-20 standard. In detail, all transfer operations (transfer, transferFrom) do not yield a bool variable and thus will cause all require checks to fail on these operations to fail.
The asset will be unsupported in the market and potentially locked if offers are made with it.
We advise the codebase to make use of the SafeERC20 library by OpenZeppelin which opportunistically evaluates the yielded bool variable of the transfer operations only if it exists, ensuring maximum asset and EIP standard compatibility.
Issue is deducible by inspecting the relevant lines referenced in the issue and cross-referencing the code with the transfer and transferFrom implementations of the deployed USDT asset.
Manual inspection of the codebase.
#0 - bghughes
2022-06-04T01:17:55Z
Duplicate of #316
🌟 Selected for report: kenzo
Also found by: 0x1f8b, 0xsomeone, Dravee, IllIllI, MaratCerby, berndartmueller, cryptphi, xiaoming90
42.6857 USDC - $42.69
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
As the BathToken is meant to be agnostic and introduce an asset to the Rubicon market, support for a wide array of the top cryptocurrencies is expected. One of the top ones is USDT (Tether), which the codebase is incompatible with due to the unique way the USDT approvals operate. In detail, a non-zero approval of USDT is only allowed when the existing approval is zero and vice versa. This causes iterative approve instructions to lead to a fatal failure due to the require check present in USDT and in particular line 205.
The USDT asset will cause any consequent approvals to fail (either due to a proxy upgrade or a call to approveMarket), potentially locking funds within the system and causing general misbehaviours.
We advise the code to perform an approval of 0 prior to any non-zero approval via a utility function to ensure token support of the top cryptocurrencies is satisfied.
Issue is deducible by inspecting the relevant lines referenced in the issue, making note of the non-zero approve operations and cross-referencing the code with the USDT implementation linked above.
Manual inspection of the codebase.
#0 - bghughes
2022-06-03T22:32:41Z
See #100
162.6494 USDC - $162.65
https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L595 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L598
The _withdraw mechanism of the BathToken does not conform to the Checks-Effects-Interactions pattern as it distributes bonus token rewards prior to the redemption (i.e. burning) of shares. While this in and of itself is an issue in the case of re-entrant EIP-777 tokens, it is an even more valid vulnerability as the system is expected to handle native asset rewards and thus cause re-entrancies via native asset transfers as well like the VestingWallet implementation and corresponding release implementation.
The re-entrant call can lead to redemptions beyond the first to have unfair evaluations as the original shares will not have exited the system at that point in time.
We advise the code to conform to the CEI pattern by performing any external calls (such as reward distributions) after the shares have been burned and all state variables have been properly updated. As an additional security measure, we advise the non-reentrant modifier by OpenZeppelin's ReentrancyGuard contract to be applied throughout the codebase.
Issue is deducible by inspecting the relevant lines referenced in the issue, making note of the distributeBonusTokenRewards function execution prior to the _burn operation.
Manual inspection of the codebase.
#0 - bghughes
2022-06-03T23:37:24Z
Good recommendation to use Reentrancy Gaurd and Duplicate of #283
🌟 Selected for report: 0xsomeone
892.4522 USDC - $892.45
https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L844 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L857 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L883 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L898 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L927 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L951
The referenced lines all perform unsafe multiplications using the unitary denominations of either 1 ether (1e18) or 10**9 (1e9), both of which can easily lead to overflows when used as a multiplier for large amounts of assets.
Purchasing and selling amounts will be improperly fulfilled as well as improperly tracked as "sold out" / "bought out".
We advise the codebase to make use of the mul operation exposed by the DSMath library already incorporated into the codebase to guarantee all operations are performed safely and cannot overflow.
Issue is deducible by inspecting the relevant lines referenced in the issue and making note of the raw multiplication (*) operations performed.
Manual inspection of the codebase.
#0 - HickupHH3
2022-06-21T03:58:21Z
In order to overflow, you would need pay_amt to exceed type(uint256).max / 1e18, which would be highly unlikely with majority of the ERC20 tokens.
Possibly arguable with ERC20 tokens of much higher decimals though. Considering that the product is an open orderbook, I'll let this issue stand.