Platform: Code4rena
Start Date: 01/09/2023
Pot Size: $36,500 USDC
Total HM: 4
Participants: 70
Period: 6 days
Judge: kirk-baird
Id: 281
League: ETH
Rank: 13/70
Findings: 2
Award: $778.38
🌟 Selected for report: 0
🚀 Solo Findings: 0
771.2966 USDC - $771.30
Once an account is blacklisted or sanctioned the rUSDY funds it holds can not be burned
According to the team the rUSDY.burn() function will be used “in cases in which a user has USDY but is legally not allowed to own it” so for example if he got it from hacking someone else’s wallet. Normaly in case of a hack the rUSDY of the suspicious account would not be burned right away but the account would be put on the blocklist first until all details are clarified.
The problem is that the burn() function calls _burnShares which calls _beforeTokenTransfer. _beforeTokenTransfer checks if the accounts involved in the transaction are on the blocklist / sanction list and reverts if they are. This means that if the account is on one of the two list, it is not possible to burn its shares. So before burning the shares of the suspicious account, the admins needs to remove the account from the blocklist/sanction list. Since removing the account from the lists and burning the shares are two different transactions, this opens up a possibility for the suspicious account to front run the burning transaction and move his rUSDY to an other account that is not on any of the list. Then he can burn the rUSDY and swapping the received USDY to any other coin and escape with the hacked funds.
Manual review
When calling rUSDY.burn(), burn the shares of the contract directly without calling _burnShares. This way the transaction does not call _beforeTokenTransfer and does not revert if the account the shares should be burned from is on the blocklist or the sanction list.
Timing
#0 - c4-pre-sort
2023-09-07T22:01:09Z
raymondfam marked the issue as primary issue
#1 - c4-pre-sort
2023-09-07T22:01:24Z
raymondfam marked the issue as sufficient quality report
#2 - tom2o17
2023-09-11T16:31:43Z
Can I not assume that the guardian can batch execute transactions? Given that the guardian will also have the ability to add/remove from blocklist can I not assume that the guardian can batch:
blocklist.removeFromBlocklist() rUSDY.burn() blocklist.addToBlocklist()
ifl this is not an issue considering the guardian address can execute any peripheral txns in an atomic fashion.
#3 - c4-sponsor
2023-09-11T16:31:44Z
tom2o17 (sponsor) disputed
#4 - kirk-baird
2023-09-19T09:15:09Z
This is an interesting edge case. While it may be possible for guardian to bypass this issue, if it is a smart contract that can batch transactions I see this as a potential issue.
Going to downgrade it to medium severity as there are some theoretical workarounds to this problem.
#5 - c4-judge
2023-09-19T09:15:16Z
kirk-baird changed the severity to 2 (Med Risk)
#6 - c4-judge
2023-09-19T09:16:13Z
kirk-baird marked issue #136 as primary and marked this issue as a duplicate of 136
#7 - c4-judge
2023-09-19T09:16:37Z
kirk-baird marked the issue as satisfactory
#8 - tom2o17
2023-09-26T19:29:33Z
Not to impact judging,
But to your point @kirk-baird, we are using a gnosis safe contract as the guardian, and plan to do a similar setup for the majority of tokens going forward.
Perhaps we should have made note of that in the ReadME.md 🤷🏼♀️
#9 - kirk-baird
2023-09-27T01:29:08Z
Not to impact judging,
But to your point @kirk-baird, we are using a gnosis safe contract as the guardian, and plan to do a similar setup for the majority of tokens going forward. Perhaps we should have made note of that in the
ReadME.md🤷🏼♀️
Okay thanks for clarifying, that does resolve the issue. Though, for the judging the wardens weren't aware of this so I'll consider it a valid issue.
🌟 Selected for report: adriro
Also found by: 0x6980, 0xStalin, 0xanmol, 0xmystery, 0xpanicError, Arz, Aymen0909, BenRai, Breeje, Lalanda, MohammedRizwan, Raihan, SovaSlava, Stormreckson, Udsen, ast3ros, bin2chen, castle_chain, catellatech, codegpt, dev0cloo, gkrastenov, hals, klau5, kutugu, ladboy233, matrix_0wl, nirlin, ohm, peanuts, pipidu83, sandy, wahedtalash77
7.08 USDC - $7.08
When user A wants to decrease the allowance he has given to user B and user B spends some of the allowance in the meantime, the function will revert and user B will still have some allowance that he should not have
When calling decreaseAllowance() the function checks if the amount by which the allowance should be decreased is <= the current allowance. If the amount is bigger than the current allowance, the function reverts, and the allowance is not changed. This results in the user who should not be having any allowance to still have some allowance he should not be having.
Manual review
Since the intend of user A was to take away the allowance given to user B consider, instead of reverting, to set the allowance for user B to 0 if the decrease in allowance exceeds the current allowance. This prevents use B from spending any more of user A’s tokens and saves user A from the need to call reduceAllowance again and pay gas again.
ERC20
#0 - c4-pre-sort
2023-09-08T17:17:05Z
raymondfam marked the issue as low quality report
#1 - c4-pre-sort
2023-09-08T17:17:28Z
raymondfam marked the issue as duplicate of #52
#2 - c4-judge
2023-09-19T10:48:47Z
kirk-baird marked the issue as unsatisfactory: Invalid
#3 - BenRai1
2023-09-25T13:17:24Z
I think this is not a dublicat of https://github.com/code-423n4/2023-09-ondo-findings/issues/52. Would be great if you could take a second look here @kirk-baird
#4 - c4-judge
2023-09-25T22:45:06Z
kirk-baird marked the issue as not a duplicate
#5 - c4-judge
2023-09-25T22:45:14Z
kirk-baird removed the grade
#6 - kirk-baird
2023-09-25T22:51:02Z
Agreed with @BenRai1 this issue is different to #52 in that it covers a case where there allowance is partially front-run to prevent decreasing.
I don't believe this is sufficient to qualify for medium since the user is already able to front-run the change of allowance and spend all of the funds. However, this would be an unpleasant edge case for the allower.
@tom2o17 worth considering patching this case.
#7 - c4-judge
2023-09-25T22:51:06Z
kirk-baird changed the severity to QA (Quality Assurance)
#8 - c4-judge
2023-09-25T22:57:37Z
kirk-baird marked the issue as grade-b