Open Dollar - Stormreckson's results

Lines of code

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

Vulnerability details

Safe owners can allow other users have access to their safes, the allowed users have access to functions such as: -AllowSAFE -modifySAFECollateralization -transferInternalCoins -transferCollateral -transferCollateral -quitSystem -enterSystem -moveSAFE -addSAFE -removeSAFE -protectSAFE The access to this function allows allowed _usr to be able to modify the SAFE.

Impact

Allowed users can modify the SAFE by calling the various functions. Owners can allow trusted accounts have access to the SAFE, the allowed _usr can transfer the SAFE collateral and internal coins to a _dst of their choice without owners consent leading to loss of funds for the Owner if the allowed _usr turns out to be malicious.

Proof of Concept

  function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
    address _owner = _safeData[_safe].owner;
    safeCan[_owner][_safe][_usr] = _ok;
    emit AllowSAFE(msg.sender, _safe, _usr, _ok);
  }

The allowSAFE() is called by the owner and approved users to allow _usr have access to the safe by setting the _ok to 1 which adds the user to the safeCan mapping and 0 which removes the user form the mapping.

The modifier in the function checks of the caller is the owner of the safe or an allowed user

  modifier safeAllowed(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
    _;
  }

Since a user In the safeCan mapping is allowed to call the allowSafe function and the SAFE owner previously allowed them access a malicious user can call allowSAFE on an account controlled by them granting themselves additional access to the SAFE, The malicious user can now call transferCollateral or and transferInternalCoins will sending them to a _dst account they control.

  function transferCollateral(bytes32 _cType, uint256 _safe, address _dst, uint256 _wad) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).transferCollateral(_cType, _sData.safeHandler, _dst, _wad);
    emit TransferCollateral(msg.sender, _cType, _safe, _dst, _wad);
  }

  function transferInternalCoins(uint256 _safe, address _dst, uint256 _rad) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).transferInternalCoins(_sData.safeHandler, _dst, _rad);
    emit TransferInternalCoins(msg.sender, _safe, _dst, _rad);
  }

Tools Used

Vscode

Recommended Mitigation Steps

I think we should allow the allowSAFE function be only called by the SAFE owner this way the SAFE owner can monitor which users are added to the safeCan .

Assessed type

Other

Lines of code

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

Vulnerability details

users can transfer safe ownership by calling _afterTokenTransfer which then calls transferSAFEOwnership on the ODSafeManager.sol contract which then makes the transfer of safe ownership, However during the transfer the delegated users by the previous owner aren't revoked and they still have access to the safe of the new owner.

Impact

The new SAFE owner can lose all their funds to the previous delegated users.

Proof of Concept

During the transfer of SAFE ownership the SAFEData is retrieved which contains the information about the SAFE

    SAFEData memory _sData = _safeData[_safe];

In theSAFEData the _usrSafes and _usrSafesPerCollateral are retrieved this is done to remove the transfered SAFE from the _src owner nappings and adding it to the _dst mappings then assigning the new owner _dst as the owner of the SAFE

    _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;

The _safeData is no containing the _safeCan mapping of the previous owner which maps the delegated users to a SAFE

  mapping(address _owner => mapping(uint256 _safeId => mapping(address _caller => uint256 _ok))) public safeCan;

This mapping is updated when owners or the delegated users call allowSAFE() to update the mapping granting the _usr access to the safeId

  function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
    address _owner = _safeData[_safe].owner;
    safeCan[_owner][_safe][_usr] = _ok;
    emit AllowSAFE(msg.sender, _safe, _usr, _ok);
  }

During the transfer of SAFE ownership this mapping is ignored. Which leaves the delegated _usr access intact and they can steal the funds of the new owner. By calling transferCollateral and or transferInternalCoins to a specified _dst account due to the access control which allows allowed SAFE users access to the function

function transferCollateral(bytes32 _cType, uint256 _safe, address _dst, uint256 _wad) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).transferCollateral(_cType, _sData.safeHandler, _dst, _wad);
    emit TransferCollateral(msg.sender, _cType, _safe, _dst, _wad);
  }

  function transferInternalCoins(uint256 _safe, address _dst, uint256 _rad) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).transferInternalCoins(_sData.safeHandler, _dst, _rad);
    emit TransferInternalCoins(msg.sender, _safe, _dst, _rad);
  }

Tools Used

VsCode

Recommended Mitigation Steps

The _safeData should include the _safeCan mapping of owners and remove the delegated users access to the SAFEs before transferring SAFE ownership.

Assessed type

Context

1- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L94-L95

mint() in vault721 isn't checking if the safeId already exist before minting, it is recommended to include a check for duplicate safeId even if there's a nonce increment it adds additional safe guard.

2- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105-L106

Permission Level Validation Issue in SafeCan Mapping

The mapping is designed to store the  _ok  value, which represents permission levels (0 for not allowed and 1 for allowed).

•The allowSAFE() does not check if the user had been added to the safeCan mapping. It is advised to include a check if the _usr being added already exist in the mapping so that a _usr won't be added twice causing issues when trying to change _ok to 0.

•Also there's no explicit validation or checks in the allowSAFE function, allowing the possibility of assigning values outside the valid range (0 or 1). This could lead to unexpected behavior and potential security risks. Assigning a value of 2 or other invalid values to  _ok without proper handling or validation could lead to unpredictable behavior, potentially compromising the security and functionality of the system.

##FIX Include proper checks and validation in the relevant function to enforce the valid range of the  _ok  parameter (0 or 1). Reject any values outside this range to ensure consistent and expected behavior. Also check if the user had already been added.

3- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/SAFEEngine.sol#L113

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/SAFEEngine.sol#L125-L126

The functions transferCollateral and transferInternalCoins aren't checking if _dst address is valid not a dead address. Including this check will prevent collateral and coins not to be lost forever.

4- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L189-L190

Users can choose to call quitSystem if they want to remove a safe from the system, this will send the safeData to a specified _dst account by calling ISAFEEngine

      _sData.collateralType, _sData.safeHandler, _dst, _deltaCollateral, _deltaDebt
    );

However there's no check if the _dst is a null or zero address transfering the safeData to a zero address leading to loss of safe.

5- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L217-L218

The moveSAFE function is used to move the safe from _safeSrc to _safeDst the function checks if the _src and _dst collateral types match but doesn't check if the _safeDst mapping exist. This check should be included so users won't move safe to a non existent safe destination.

6- https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L235-L236

addSAFE function is called when new safes are being added to the system by a user, the function adds a new safe to the user safes mapping.

There's no check indicating if the _safeId exist in the mapping before adding it, this can lead to addition of the same _safe which might cause issues when trying to access the safe due to mapping ordering. Include a check if the safe had been added before adding the new one.