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#L235 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L242

Vulnerability details

Impact

Users can add SAFE instances that do not belong to them, and once added, there is no way to remove them.

Proof of Concept

The absence of access control in the addSAFE() function enables users to add SAFE instances that belong to others.

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

Once a SAFE has been added, it becomes impossible to remove it because the removeSAFE() function is protected by the safeAllowed modifier. Only the owner or authorized individuals can call this function.

  function removeSAFE(uint256 _safe) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    _usrSafes[_sData.owner].remove(_safe);
    _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

  function addSAFE(uint256 _safe) external {
    SAFEData memory _sData = _safeData[_safe];
+   require(msg.sender == _sDate.owner;
    _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)