`ODSafeManager.transferSAFEOwnership()` function doesn't revoke old permission leading to manipulate

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

ODSafeManager.transferSAFEOwnership() function delete safeCan of the previous owner. Because of that, when one of the old owner owns the NFTs again, there can be a risk of now-malicious old permission that can execute actions to the safe.

Proof of Concept

Imagine this scenario:

• User A have a Safe/NFT X
• User A give the safe permission of NFT X using allowSAFE() to user B to do something
• User A now sell the NFT X, user C now is the owner of the NFT X
• After some months, A buys NFT X from the marketplace
• Because A is now the owner of the NFT X, user B now has the permission to the NFT X
• But user B is now not trusted by user A, B maliciously executes actions to the NFT X

  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;

    <@@ NOTICE here that's the function doesn't `delete safeCan[_sData.owner][_safe]` here

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

Tools Used

Manual Review

Recommended Mitigation Steps

  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;

+   delete safeCan[_sData.owner][_safe];

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

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #41

[c4-pre-sort]

raymondfam marked the issue as duplicate of #142

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-judge]

MiloTruck marked the issue as satisfactory