Open Dollar - lsaudit's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L58 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/UniV3Relayer.sol#L64

Vulnerability details

Impact

For the tokens with decimals greater than 18, CamelotRelayer.sol and UniV3Relayer.sol will revert because of the underflow.

Protocol performs normalization only for tokens with decimals <= 18. It is assumed that the max number of decimals for each ERC-20 token is 18. However, this assumption is wrong. There are some ERC-20 tokens with decimals > 18. For example: YAMv2 has 24 decimals. Protocol won't work with that tokens, because it will underflow and revert.

I've decided to evaluate this issue as Medium. Since protocol performs normalization for tokens with decimals <=18, I've assumed that it's designated to work with every valid ERC-20 token. However, there are some valid ERC-20 tokens (with > 18 decimals) which won't work with the protocol - thus I've evaluated this issue as Medium.

Moreover, during the previous Code4rena contests, the similar findings had been evaluated as Medium:

• https://code4rena.com/reports/2023-01-astaria#m-28-lack-of-support-for-erc20-token-that-is-not-18-decimals
• https://code4rena.com/reports/2022-04-backd#m-08-_decimalmultiplier-doesnt-account-for-tokens-with-decimals-higher-than-18
• https://code4rena.com/reports/2022-06-connext#m-11-tokens-with-decimals-larger-than-18-are-not-supported
• https://code4rena.com/reports/2021-06-tracer#m-04-underflow-problems-occurring-when-a-token-has-18-decimals

Proof of Concept

The issue occurs in two instances:

File: CamelotRelayer.sol

 multiplier = 18 - IERC20Metadata(_quoteToken).decimals();

File: UniV3Relayer.sol

multiplier = 18 - IERC20Metadata(_quoteToken).decimals();

Whenever _quoteToken contains more than 18 decimals, there will be revert, because of the underflow.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider checking scenario when decimals are greater than 18 and normalize the value properly.

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

Function transferSAFEOwnership() is only callable by Only Vault721: require(msg.sender == address(vault721), 'SafeMngr: Only Vault721'); and its purpose is to give the safe ownership to a dst address. However, transferSAFEOwnership() does not reset safeCan mapping which is used by safeAllowed modifier.

This leads to some security issues. The modifier safeAllowed is used to verify if user has permission to the safe. Multiple of functions which perform critical operations on the safe use this modifier to verify is user has permission to call that operation on a given safe.

Since transferSAFEOwnership() transfer ownership to a different address, it should also reset all previously set privileges by the previous owner. However, safeCan mapping is not being reset in transferSAFEOwnership()

Proof of Concept

Firstly, let's take a look how safeAllowed modifier is defined:

File: ODSafeManager.sol

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

For this PoC, let's consider user A, who's the owner of safe 123. He gives access to the safe to X, Y, so he calls: allowSAFE(123, X, 1), allowSAFE(123, Y, 1);

File: ODSafeManager.sol

 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);
  }

After calling this function, safeCan mapping looks like this: safeCan[A][123][X] = 1 and safeCan[A][123][Y] = 1.

Now, there's a call to transfer safe ownership to _dst B: transferSAFEOwnership(123, B)

File: ODSafeManager.sol

function transferSAFEOwnership(uint256 _safe, address _dst) external {
    require(msg.sender == address(vault721), 'SafeMngr: Only Vault721');

    if (_dst == address(0)) revert ZeroAddress();
    SAFEData memory _sData = _safeData[_safe];
    if (_dst == _sData.owner) revert AlreadySafeOwner();

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

    emit TransferSAFEOwnership(msg.sender, _safe, _dst);
  }

It modifies only _usrSafes, _usrSafesPerCollat and _safeData mappings and does not modify safeCan mapping. After transferSAFEOwnership(123, B) call, those mappings look like this:

_usrSafes[B] = [123]
_usrSafesPerCollat[B][_sData.collateralType] = [123]
_safeData[123].owner = B;

safeCan[A][123][X]  // notice this is from previous allowSAFE() call
safeCan[A][123][Y]  // notice this is from previous allowSAFE() call

Now, at some point in the future, the safe goes back to user A. Function transferSAFEOwnership() is being called once again: transferSAFEOwnership(123, A).

User A get access to the safe. However, he does not remember that he previously sets access to users X and Y. Those mappings:

safeCan[A][123][X]
safeCan[A][123][Y]

are still there - meaning that X and Y has still access to that safe, even though the safe was just transferred back to A and safe's permissions should be cleared.

Now, when user X or Y will try to call any function with safeAllowed modifier - they will be allowed, since safeAllowed will return true for them, because X and Y are still in safeCan mapping.

Tools Used

Manual code review

Recommended Mitigation Steps

Whenever transfer ownership - make sure to reset safeCan[old_owner][safe] mapping.

Assessed type

Access Control

[QA-1] Lack of checking of address(0) in constructor in SAFEHandler.sol

File: SAFEHandler.sol

ISAFEEngine(_safeEngine).approveSAFEModification(msg.sender);

There's no check if _safeEngine is address(0).

[QA-2] Missing check for contract existence in ODProxy.sol

File: ODProxy.sol

(_succeeded, _response) = _target.delegatecall(_data);

If there's no contract at _target, the _target.delegatecall(_data) will return true. Check for the contract existence before delegate calling to the _target.

[QA-3] OWNER is immutable in ODProxy.sol, which means it won't be possible to change it in the future

When, for any reason - there would be need to change the owner of ODProxy.sol - it won't be possible, since OWNER is set as immutable variable.

File: ODProxy.sol

address public immutable OWNER;

[QA-4] Unify order of parameters in ODProxy.sol

File: ODProxy.sol

function quitSystem(uint256 _safe, address _dst) external safeAllowed(_safe) handlerAllowed(_dst) {

(...)

function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe) {

Functions quitSystem() and enterSystem() - are related to the similar funcionality. First one allows to quit system, while the second one - enter it. The parameters order should be unified for them. In current implementation: for quitSystem() _safe is the 1st parameter, while _dst the 2nd one. For enterSystem(), _safe is the 2nd parameter, while _src is the 1st one. Since both quitSystem() and enterSystem() operates on _safe - consider using it as 1st parameter in both functions:

quitSystem(uint256 _safe, address _dst)
enterSystem(uint256 _safe, address _src)

[QA-5] Unify the order of modifiers in OdProxy.sol

File: ODProxy.sol

function quitSystem(uint256 _safe, address _dst) external safeAllowed(_safe) handlerAllowed(_dst) {

(...)

function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe) {

Consider unifying the order of modifiers to be the same in functions, which share related functionality. Firstly, use safeAllowed(_safe), then handlerAllowed(_dst):

function quitSystem(uint256 _safe, address _dst) external safeAllowed(_safe) handlerAllowed(_dst) {

(...)

function enterSystem(address _src, uint256 _safe) external safeAllowed(_safe) handlerAllowed(_src)  {

[QA-6] Lack of checking of address(0) in constructor in ODSafeManager.sol

File: ODProxy.sol

vault721 = IVault721(_vault721)

While constructor verifies that _safeEngine is not address 0, it does not verify if _vault721 is not address 0.

[QA-7] Lack of checking of address(0) in functions in ODSafeManager.sol

• function allowHandler() File: ODProxy.sol

handlerCan[msg.sender][_usr] = _ok;

There's no check if _usr is not address(0).

[QA-8] Lack of event emission in ODSafeManager.sol

Functions addSAFE() and removeSAFE() do not emit events when safe is added/removed.

File: ODProxy.sol

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

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

[QA-9] Loop over unbounded array in ODSafeManager.sol

New safe can be added to _usrSafes by calling addSAFE():

File: ODSafeManager.sol

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

This means, that there's no limit of number of safes which can be added. User can add addSAFE() with new safe as many times he/she wants. This means, that _usrSafes and _usrSafesPerCollat can contain a lot of elements.

In File: ODSafeManager.sol

function getSafesData(address _usr)
    external
    view
    returns (uint256[] memory _safes, address[] memory _safeHandlers, bytes32[] memory _cTypes)
  {
    _safes = _usrSafes[_usr].values();
    _safeHandlers = new address[](_safes.length);
    _cTypes = new bytes32[](_safes.length);
    for (uint256 _i; _i < _safes.length; _i++) {
      _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler;
      _cTypes[_i] = _safeData[_safes[_i]].collateralType;
    }
  }

we are looping over _usrSafes[_usr]. This means, that if there are a lot of added safes, function will revert with out of gas error. It should be a way for getSafesData() to display limited set of safes, to avoid that revert. E.g. re-implement a function in a way, which will allow to choose how many safes we want to return (and the starting point, e.g. we want to get limit set starting at index offset). One way of solving this problem would be re-implementing getSafesData() by adding offset and limit parameters to it.

function getSafesData(address _usr, uint _offset, uint _limit)
    external
    view
    returns (uint256[] memory _safes, address[] memory _safeHandlers, bytes32[] memory _cTypes)
  {
    _safes = _usrSafes[_usr].values();
    _safeHandlers = new address[](_safes.length);
    _cTypes = new bytes32[](_safes.length);
    require(_safes.length >= _offset + _limit, "Out of bounds error!");
    for (uint256 _i = _offset; _i < _limit; _i++) {
      _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler;
      _cTypes[_i] = _safeData[_safes[_i]].collateralType;
    }
  }

[QA-10] Lack of event emission in Vault721.sol

Function which set/change important protocol's parameters should emit events. Below, there's a list of functions from Vault721.sol which does not emit events:

• updateNftRenderer
• updateContractURI
• setSafeManager
• setNftRenderer

[N-1] Invalid comment in ODSafeManager.sol

File: ODSafeManager.sol

  // Give the safe ownership to a dst address.
  function transferSAFEOwnership(uint256 _safe, address _dst) external {

Give the safe ownership to a dst address. should be changed to Give the safe ownership to a _dst address., since function transferSAFEOwnership() has _dst (not dst) parameter.

[N-2] Make comments in UniV3Relayer.sol more specific

File: UniV3Relayer.sol

 Method will return invalid if the pool doesn't have enough history

There's no explanation what "invalid" means. It turns out, that when pool doesn't have enough history, function will return: (0, false). Whenever commenting a function behavior, be as specific as possible. Word "invalid" is too ambiguous in this context.

[N-3] Abide to Checks Effects Interactions pattern in AccountingEngine.sol

File: AccountingEngine.sol

    safeEngine.settleDebt(_rad);
    totalOnAuctionDebt -= _rad;

Reduce the totalOnAuctionDebt before calling safeEngine.settleDebt().

This does not pose any security risk, especially, that safeEngine.settleDebt() does not use totalOnAuctionDebt variable. Nonetheless, coding with keeping Checks Effects Interactions pattern in mind is good security practice.