Open Dollar - Tendency's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/CommonActions.sol#L85-L101 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/CommonActions.sol#L107-L121

Vulnerability details

Impact

When locking collateral in safeEngine or when exiting collateral tokens from the safeEngine contract, the internal functions CommonActions::_joinCollateral and _exitCollateral are called respectively during execution. The problem with this function is, while attempting to convert the input wad amount from 18 decimal precision to the decimal precision of the collateral token, the current implementation, assumes the collateral token decimals to be <= 18 decimal precision. When this is not the case, there will always be an underflow revert.

As confirmed by the sponsor: " OPenDollar may accept collateral tokens with more than 18 decimal precision, so should be fixed."

Proof of Concept

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/CommonActions.sol#L85-L101

Tools Used

Manual Review

Recommended Mitigation Steps

Consider creating a separate functionality that handles token decimal conversions of all types. You can reference: https://github.com/IPOR-Labs/ipor-protocol/blob/main/contracts/libraries/math/IporMath.sol

Also CamelotRelayer should be fixed to allow creating of a relayer for tokens with more than 18 decimal precision, UniV3Relayer as well.

Assessed type

Decimal

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L189-L202 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L205-L215

Vulnerability details

Impact

ODSafeManager::allowHandler is meant to be called by a safe Handler to give permission to users before they can perform any action that modifies the safe handler's state in SAFEEngine contract.

The problem here is, that while this function is meant to be called by a safe safeHandler contract, there is no available functionality for this in SAFEHandler contract.

contract SAFEHandler {
  /**
   * @dev    Grants permissions to the SAFE manager to modify the SAFE of this contract's address
   * @param  _safeEngine Address of the SAFEEngine contract
   */
  constructor(address _safeEngine) {
    ISAFEEngine(_safeEngine).approveSAFEModification(msg.sender);
  }
}

This will make ODSafeManager::enterSystem and ODSafeManager::quitSystem functions to be completely inoperable since there is no way for the safeHandler to give out permissions.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

A possible fix here will be to add a functionality in SAFEHandler that calls the ODSafeManager::allowHandler function, this functionality should be restricted to the safe owner only. Will also add, as highlighted in another of my issues, if ODSafeManager::moveSAFE function is used, while enterSystem() and quitSystem() functions are removed, then allowHandler() function and handlerAllowed modifier can be removed. This way, the safe owner can give out permissions via allowSAFE().

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

ODSafeManager::allowSAFE is a function called by safe owners to allow/disallow a user address to manage their safe.

The problem with the current implementation is, that when the safe owner gives permission to a user to manage their safe, the user can still give another user permission to manage the owner's safe. For example: Alice owns safe A, Alice then gives Bob permission to manage her safe, the current implementation allows Bob to further give Charlie permission to manage Alice's safe, and so on.

As confirmed by the sponsor:

 I agree that it should be limited to only the owner

This kind of functionality should be restricted to the owner only. If Alice trusts Bob to manage her safe, Bob shouldn't be allowed to give others the same privilege.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Just like ODSafeManager::allowHandler, ODSafeManager::allowSAFE should be restricted to the owner only.

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

Assessed type

Error

[L01] Anyone can Add any Safe Among Their Owned Safes

ODSafeManager::addSAFE function allows anyone to add any safe amongst their owned safes.

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

RECOMMENDATION

Only a safe owner should be allowed to add a safe to their safe list.

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