Users can prevent the burning of their tokens if their USDY is supposed to be taken/seized by the protocol.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L678

Vulnerability details

Impact

• The protocol could not enforce their policies over users who are not allowed to own USDY because the users can prevent the burning of their tokens by either unwrap their rUSDY or move it to different accounts.

Proof of Concept

• The purpose of the rUSDY:burn() can be thought of as a mean for the protocol to seize funds from accounts who are not allowed to own USDY (see sponsor's explanation).
  

• The problem with the existing logic is that the account must not be blacklisted/sanctioned and it has to be allowed, the reason is because to burn the shares, the _burnShares() is called, and in this function, the _beforeTokenTransfer() function is called, inside the _beforeTokenTransfer() is verified if the account is blacklisted/sanctioned or not allowed to execute operations.

  • So, if the account must be allowed in order for the burning operation to be executed, that means that the account's owner can also execute transfers of rUSDY to another accounts and unwrap the rUSDY for USDY.

    • burn() => _burnShares() => _beforeTokenTransfer()

function _beforeTokenTransfer(
  address from,
  address to,
  uint256
) internal view {
  // Check constraints when `transferFrom` is called to facliitate
  // a transfer between two parties that are not `from` or `to`.
  if (from != msg.sender && to != msg.sender) {
    require(!_isBlocked(msg.sender), "rUSDY: 'sender' address blocked");
    require(!_isSanctioned(msg.sender), "rUSDY: 'sender' address sanctioned");
    require(
      _isAllowed(msg.sender),
      "rUSDY: 'sender' address not on allowlist"
    );
  }

  //@audit-info => When burning, it will check if the account is not blacklisted/sanctioned and it is allowed, if the account meets this criteria, it also means that the owner can do transfers and unwrap rUSDY
  if (from != address(0)) {
    // If not minting
    require(!_isBlocked(from), "rUSDY: 'from' address blocked");
    require(!_isSanctioned(from), "rUSDY: 'from' address sanctioned");
    require(_isAllowed(from), "rUSDY: 'from' address not on allowlist");
  }

  if (to != address(0)) {
    // If not burning
    require(!_isBlocked(to), "rUSDY: 'to' address blocked");
    require(!_isSanctioned(to), "rUSDY: 'to' address sanctioned");
    require(_isAllowed(to), "rUSDY: 'to' address not on allowlist");
  }
}

• Let's suppose the scenario where the protocol has determined that AccountA is not allowed to own USDY, the account currently owns 250 USDY. So, the protocol calls the burn() to burn the 250 USDY(shares) from the AccountA. In the mempool it is already a tx from the AccountA's owner to unwrap 50 USDY.
  • So, the tx of AccountA's owner is executed first and then is executed the tx from the protocol, as a result, the tx from the protocol to burn the 250USDY fails because now the AccountA only have 200USDY, the owner of the AccountA realizes that the protocol is trying to burn his USDY and proceeds to unwrap all the reamining balance or transfer it to different accounts.
  • The result is that the protocol was not able to seize the funds from an account that is not allowed to hold USDY.

Tools Used

Manual Audit

Recommended Mitigation Steps

• Refactor the logic in the _burnShares() so that depending on the reason to burn shares will check or not that the account is blacklisted/sanctioned.
• If the protocol has determined that an account is not allowed to hold funds and will proceed to seize them, then, that account should be either be blacklisted/sanctioned or notAllowed to perform any operation that would prevent the seizing of their tokens.
• Add a new flag to the _burnShares() that will indicate to the function if needs to check the account status (blacklisted/sanctioned) or not.
  • If the burning is coming from the burn(), the account check should not be done, and the burning should be executed anyways, but if the burning is coming from another function, like the unwrap(), then the account check should be done.

function burn(
  address _account,
  uint256 _amount
) external onlyRole(BURNER_ROLE) {
  uint256 sharesAmount = getSharesByRUSDY(_amount);

  //@audit-info => Set to true the seizing flag, so the account check is skipped, because the account should already be blacklisted/sanctioned!
  _burnShares(_account, sharesAmount,true);

  ...
}

function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
  ...

  //@audit-info => Set to false the seizing flag, so the account check is made!
  _burnShares(msg.sender, usdyAmount,false);
  
  ...

}


function _burnShares(
  address _account,
  uint256 _sharesAmount,
  bool seizing
) internal whenNotPaused returns (uint256) {
  ...

  //@audit-info => Check the account status if the burning comes from any function other than the burn() | If the flag is set to false, then, the account check must be done!
  if(!seizing) _beforeTokenTransfer(_account, address(0), _sharesAmount);

  ...

}

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as duplicate of #313

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[c4-judge]

kirk-baird removed the grade

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate of #136

[c4-judge]

kirk-baird marked the issue as satisfactory