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: 37/99
Findings: 2
Award: $214.69
🌟 Selected for report: 1
🚀 Solo Findings: 0
162.6494 USDC - $162.65
If a token with callback capabilities is used as a token to vested, then a malicious beneficiary may get the vested amount back without waiting for the vesting period.
In the function release, line (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L87), there’s no modifier to stop reentrancy, in the other contracts it would be the synchronized modifier. If a token could reenter with a hook in a malicious contract (an ERC777 token, for example, which is backwards compatible with ERC20), released token counter array (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L116) wouldn’t be updated, enabling the withdrawal of the vested amount before the vesting period ends. A plausible scenario would be:
A malicious beneficiary contract B calls the release() function with itself as the recipient, everything goes according to the function, and transfer and callback to the malicious beneficiary contract happens.
Contract B contains tokensReceived(), a function in the ERC777 token that allows for callback to the victim contract as you can see here https://twitter.com/transmissions11/status/1496944873760428058/ (This function also can be any function that is analogous to a fallback function that might be implemented in a modified ERC20. As it can be seen, any token that would give the attacker control over the execution flow will suffice.)
Inside the tokensReceived() function, a call is made back to the release function.
This steps are repeated until vested amount is taken back.
This allows for the malicious beneficiary contract to redeem the vested amount while bypassing the vesting period, due to the released token counter array (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L116) which controls how many tokens are released (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L101) being updated only after the transferring of all tokens occurs. As this is the case, malicious beneficiary can get the usual amount that they could withdraw at the time indefinite amount of times (as result of released in line 101 will be 0), thus approximately getting all of their vested amount back without waiting for the vesting period. (fees not included).
There's also precedents of similar bugs that reported, as seen here: https://github.com/code-423n4/2022-01-behodler-findings/issues/154#issuecomment-1029448627
Manual code review, talks with dev
Consider adding a mutex such as nonReentrant, or the synchronized modifier used in the other contracts.
Implement checks-effects-interactions pattern.
#0 - bghughes
2022-06-03T20:01:17Z
This one should probably be a high priority, adding disagreement with severity
#1 - HickupHH3
2022-06-18T06:33:43Z
From what I gather, bonusTokens can be any ERC20 token (network incentives/governance tokens), which by extension, means ERC777 tokens are possible too.
Even if not, I suppose one could spin up a malicious bonus token to specifically target the BathBuddy contract.
Medium severity because there isn't a loss nor stolen rewards from others, merely bypassing the vesting, which is an impact on protocol functionality.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
🌟 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.0351 USDC - $52.04
In the line (https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L2), there’s a floating Pragma. Contracts should be deployed with the same compiler version and flags that they have been tested with to avoid using an outdated compiler version that might introduce bugs.
In the line (https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/TokenWithFaucet.sol#L6) an import declaration is commented out, and it should be removed.