rUSDY Funds from blacklisted or sanctioned accounts can not be burned when calling rUSDY.burn()

[code423n4]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L672-L683

Vulnerability details

Impact

Once an account is blacklisted or sanctioned the rUSDY funds it holds can not be burned

Proof of Concept

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.

Tools Used

Manual review

Recommended Mitigation Steps

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.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[tom2o17]

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.

[c4-sponsor]

tom2o17 (sponsor) disputed

[kirk-baird]

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.

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)

[c4-judge]

kirk-baird marked issue #136 as primary and marked this issue as a duplicate of 136

[c4-judge]

kirk-baird marked the issue as satisfactory

[tom2o17]

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 🤷🏼‍♀️

[kirk-baird]

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.