c4-submissions commented 1 year ago

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L73

Vulnerability details

Impact

Malicious User can show false proof of ownership of a safe by adding safes to his account that doesnot actually belongs to him/her . This opens opportunities for a malicious user to do fraudulent actvities .

Proof of Concept

The addSAFE function is for allowing the user to add a safe to their list (if it was previously removed). But insufficient validation of caller being actual owner of the safe or not gives malicious user to add any safe to his name by calling this function . The actual ownership of a safe is listed in _safeData mapping .

   function addSAFE(uint256 _safe) external {
    SAFEData memory _sData = _safeData[_safe];
    _usrSafes[msg.sender].add(_safe);
    _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
  }

Safes represent an ownership of collateral in the protocol . This NFT's are transferred by the owner and the decision of transferring this NFT's depend on the buyer and seller , protocol doesnot help in selling safe's by providing secure escrow system . By using the above issue , a malicious user can show a false ownership of a safe that actually doesnot belong to him And fraud buyers by showing that false proof of ownership .

Tools Used

Manual review

Recommended Mitigation Steps

An user should not be able to add a safe to his name if he is not the actual owner ! So, This extra checks should be added to the addSAFE function .

  function addSAFE(uint256 _safe) external {
    SAFEData memory _sData = _safeData[_safe];
+   if(msg.sender != _sData.owner) revert(); 
    _usrSafes[msg.sender].add(_safe);
    _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
  }

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #288

c4-judge commented 1 year ago

MiloTruck changed the severity to QA (Quality Assurance)