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: 26/99
Findings: 3
Award: $453.74
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
Not checking the return value from transfer means that some ERC20 implementations, which return false if a transfer fails, will not transfer the token but will have _erc20Released incremented.
Manual analysis
Use safeERC20 library from OpenZeppelin for token transfers.
#0 - bghughes
2022-06-03T20:04:30Z
Duplicate of #316
🌟 Selected for report: 0x1337
Also found by: broccolirob
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L525-L530 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L551-L567 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L448-L450
The RubiconMarket contract is meant to be upgradeable, but it inherits contracts that are not upgrade-safe. Adding new storage variables to inherited contracts will overwrite storage slots that have already been assigned on the proxy contract.
A Rubicon developer adds a new storage variables to the ExpiringMarket contract. It's an address field that holds the address of the owner who stopped the contract. That address field pushes all following storage slots down one, which pushes the initialized boolean of RubiconMarket to a slot and offset that now returns false. The Rubicon developer upgrades the RubiconMarket contract. Mallory inspects the storage variable for initialized and sees that it returns false, and so she calls the initialize method, which assigns her as the owner of the contract. Mallory now has admin privileges over critical contract functionality and can setMinSell, setBuyEnabled, setAqueductAddress, etc.
Manual analysis
Follow best practices for upgradeable contracts and provide a storage gap so that new variables can be added safely. See OpenZeppelin's suggestions around different types of storage strategies for alternative solutions to this problem.
https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
#0 - bghughes
2022-06-04T01:27:47Z
Duplicate of #67
🌟 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.0389 USDC - $52.04
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L371-L372 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L541-L552
In the code and in the documentation it's suggested that BathToken conforms to EIP4626, however, it follows neither the interface nor the specification of EIP4626.
Alice deposits a BathToken into an ERC4626 compatible contract that promises to manage it for her. BathToken does not support the specification that says the redeem function "MUST support a redeem flow where the shares are burned from owner directly where owner is msg.sender or msg.sender has ERC-20 approval over the shares of owner". Because there's no approval flow supported by BathToken.redeem the tokens end up stuck in the intermediary contract that operates under the assumption the token is a proper ERC4626 token. Alice's tokens end up getting stuck because the contract reverts every time she tries to interact with their system.
Manual Analysis
Remove all references to EIP4626 and rename functions so there's no confusion that BathToken isn't meant to be an ERC4626 vault token. Or, if it is meant to conform, ensure that all methods are implemented correctly, that it inherits from an IERC4626 interface, and that each function conforms to the proper specification.
#0 - bghughes
2022-06-03T22:59:26Z
I disagree that this does not adhere. In your analysis:
MUST support a redeem flow where the shares are burned from owner directly where owner is msg.sender or msg.sender has ERC-20 approval over the shares of owner
Where the Bath Token presently exclusively supports the former. Moreover, this would be a very low severity issue given the esoteric nature of this interaction and any implementing contracts could still handle this as they are likely the holders of said Bath Tokens.
#1 - HickupHH3
2022-06-16T07:10:35Z
I asked Alberto, fellow judge, and also co-author of the ERC4626 standard, for clarification about the wording because it is a little ambiguous.
"The text says or, so it implies that it can be just one, the intention though is that redeem must use the pattern from transferFrom."
Hence, both direct burning and from given allowance has to be supported. While the warden is correct in this case. I disagree with the POC provided because the token would've been held by the intermediary contract and would've fallen into the implemented case (owner is msg.sender).
I can't think of a scenario where there will be a loss of funds with the current implementation, because the owner of the tokens should be able to directly redeem the tokens.
Thus, as per the risk assessment, I will give it a low severity:
e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments
#2 - HickupHH3
2022-06-16T07:13:08Z
Warden does not have a QA report, keeping this as the primary issue ID for subsequent downgraded issues, if any.