Lack of event emission after updating sensitive contract addresses ' safeManager ' and ' nftRenderer '

[c4-submissions]

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L172-L174 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L179-L181

Vulnerability details

Impact

Changing sensitive addresses safeManager and nftRenderer in the vault721.sol contract without logging those events
will mislead the off-chain monitoring clients

Proof of Concept

The ODSafeManager is a critical contract that performs several operations related to 'SAFE' management. Meanwhile NFTRenderer contract is responsible for providing the metadata and other NFT related information.

So, its fair to assume that, these addresses will be monitored by off-chain clients who track the Open Dollar system.

However , the following two governance controlled functions update these addresses without logging any events.

  function _setSafeManager(address _safeManager) internal nonZero(_safeManager) {
    safeManager = IODSafeManager(_safeManager);
  }

  function _setNftRenderer(address _nftRenderer) internal nonZero(_nftRenderer) {
    nftRenderer = NFTRenderer(_nftRenderer);
  }

Now the clients would have to inspect all transactions to notice that these important addresses have been changed.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider emitting events after sensitive updates are made. This facilitates tracking and notifies off-chain clients that are following the contract’s activity.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #149

[c4-judge]

MiloTruck marked the issue as not a duplicate

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Overinflated severity

[MiloTruck]

Events-related findings are clearly non-critical, as defined in the C4 severity categorization

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Invalid