Vulnerability: User's tokens can be Stolen If the Burn Role Get Compromised or Malicious. / Contract: rUSDY.sol / Function: burn

[code423n4]

Lines of code

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

Vulnerability details

Impact

The burner can burn the shares of any account and keep the tokens of that account for himself.

Medium because: The BURNER_ROLE is a trusted entity that has the ability to burn the tokens of any account and keep the USDY tokens of that account, Although this is well-intentioned, this is a vulnerability that can cause loss of funds to the users of the protocol.

Proof of Concept

let's imagine that the user/account/contract that has the BURNER_ROLE turns malicious, the contract or the private keys get compromised, something that had already happened a lot in the Web3 space (like the Ronin Hack), due to the way the burn function works, this would give the attacker the ability to burn the shares of any user and keep the tokens for himself cause the transfer of the Usdy is sent to the msg.sender (the BURNER_ROLE).

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

function burn(address _account, uint256 _amount) external onlyRole(BURNER_ROLE) {
    uint256 sharesAmount = getSharesByRUSDY(_amount);
    _burnShares(_account, sharesAmount);
    usdy.transfer(msg.sender, sharesAmount / BPS_DENOMINATOR);
    emit TokensBurnt(_account, _amount);
}

Tools Used

Manual Code Review

Recommended Mitigation Steps

if this function was created to withdraw/burn the tokens of bad actors in the protocol it's better to create a list that registers the bad users of the protocol, so the burn function is called to burn the tokens of only the bad users in that list, so the BURNER_ROLE and the burn function do not have access/permission to burn the tokens of all users if this role get compromised.

Assessed type

Other

[raymondfam]

Centralized known issue in readme.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as duplicate of #85

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)