Users can be tricked to buy safes with status different than advertized

[c4-submissions]

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L136-L152

Vulnerability details

Impact

• When a user opens a SAFE, a "Non-Fungible Vaults" (NFV) is minted to their account, which controls the ownership and transferability of the SAFE, where the tokenId is equal to the safeId and the transferrence of a NFV equates to the transferrence of the corresponding SAFE.

• The NFV can be traded on secondry markets; once the user buys a NFV; its ownership will be transferred to him as well as the SAFE ownership.

• Each SAFE has a collateral and debt (position); so the decision of a user to buy a NFV will mainly rely on the health of the SAFE position that represents it; so he would prefer to buy healthy SAFE.

• But how could the buyer be tricked by the original SAFE owner? let's imagine the following scenario:

  1. A user wants to buy a NFV on a market place that has less generated debt and more collateral (healthy SAFE).
  2. Then this user initiated a transaction to buy this SAFE.
  3. The original owner of the SAFE sees the transaction and front-runs it with a transaction that reduces the health of the SAFE position (by generating a debt or emptying the SAFE or quitting the system or moving SAFE collateral or freeing the SAFE collateral).
  4. So the buyer will be left with an un-healthy SAFE with a state (collateral and debt) different from the original state that he sees when he first made his transaction.

• This will harm any buyer as there's no cool down period that prevents the original owner from selling the NFV after making any update on its state (generating debt or freeing collateral).

Proof of Concept

ODSafeManager.transferSAFEOwnership function

  function transferSAFEOwnership(uint256 _safe, address _dst) external {
    require(msg.sender == address(vault721), 'SafeMngr: Only Vault721');

    if (_dst == address(0)) revert ZeroAddress();
    SAFEData memory _sData = _safeData[_safe];
    if (_dst == _sData.owner) revert AlreadySafeOwner();

    _usrSafes[_sData.owner].remove(_safe);
    _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);

    _usrSafes[_dst].add(_safe);
    _usrSafesPerCollat[_dst][_sData.collateralType].add(_safe);

    _safeData[_safe].owner = _dst;

    emit TransferSAFEOwnership(msg.sender, _safe, _dst);
  }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Add a cool down period when the user generates a debt or quits the system (or any operation that decreases the SAFE health or frees its collateral), and this period to be checked if passed when the NFV is transferred to another user (bought by another user).

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #20

[c4-pre-sort]

raymondfam marked the issue as duplicate of #63

[c4-judge]

MiloTruck changed the severity to 3 (High Risk)

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[c4-judge]

MiloTruck marked the issue as grade-a