Open Dollar - COSMIC-BEE-REACH's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L205 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L112 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/SAFEHandler.sol#L11

Vulnerability details

Impact

The allowHandler function in ODSafeManager.sol contract is designed to be called by the SAFEHandler to grant permission for transferring debt and collateral to the target Safe. However, the SAFEHandler.sol contract lacks the necessary method to do so. As a result, users interested in entering the system won't be able to do so, thus disrupting the intended functionality of the contracts.Because of the design flow in allowHandler function the other intention here would have been that SafeOwner will call this function to allow permission for it's own safe but in either case this modifier will revert all the time

modifier handlerAllowed(address _handler) {
    if (msg.sender != _handler && handlerCan[_handler][msg.sender] == 0) revert HandlerNotAllowed();
    _;
  }

Proof of Concept

function allowHandler(address _usr, uint256 _ok) external {
   handlerCan[msg.sender][_usr] = _ok;
   emit AllowHandler(msg.sender, _usr, _ok);
 }

lets suppose alice owns safe A and BoB Owna safe B

• allowHandler function sets msg.sender as approver and _usr as the caller. _usr is supposed to call the 'enterSystem' typically the owner(alice's proxy) of the safe or manager of the safe.
• BOB unaware of the fact that safeHandler do not have any function to call allowHandler.Bob calls this function through it's proxy because safe is owned by Bob's proxy, to grant permission to migrating the safe (lockedCollateral, generatedDebt) from a src handler(bob's handler) to the safe handler(alice's handler).
• if call is diverted to SAFEHandler.sol it will revert all the time https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/SAFEHandler.sol#L11
• if call is diverted to ODSafeManager.sol contract then allowHandler function will set msg.sender as approver and _usr(address of the alice's proxy or address of safe manager) as the caller that will call 'enterSystem'.
• Now alice tries to call the enterSystem(address _src, uint256 _safe) with _src = bob's proxy address or original safe address of bob ,_safe = safe id of the safe owned by the alice.
• safeAllowed(_safe) modifier will not revert but handlerAllowed(_src) modifer will always revert because handlerCan[_handler][msg.sender] is 0 all the time.

modifier handlerAllowed(address _handler) {
    if (msg.sender != _handler && handlerCan[_handler][msg.sender] == 0) revert HandlerNotAllowed();
    _;
  }

Tools Used

vscode

Recommended Mitigation Steps

EITHER ADD A METHOD IN SAFEHandler CONTRACT TO CALL allowHandler OR MODIFY allowHandler FUNCTION TO REGISTER HANDLER ADDRESS MANUALLY

function allowHandler(address _usr, uint256 _ok,uint256 _safeId) external {
  - handlerCan[msg.sender][_usr] = _ok;
  + address _owner = _safeData[_safeId].owner;
  + require(_owner == msg.sender,"error");
  + address _safeHandler= _safeData[_safeId].safeHandler;
  + handlerCan[safeHandler][_usr] = _ok;
    emit AllowHandler(msg.sender, _usr, _ok);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The allowSAFE function in the ODSafeManager contract allows the owner of a SAFE to grant or revoke permissions for another user address to manage the SAFE. However, the function does not differentiate between adding and revoking permissions. This lack of differentiation means that both the owner and the current SAFE manager can use this function to either add a backup SAFE manager or remove the current/Other SAFE manager.

The impact of this vulnerability is that it allows for potentially unauthorized or unintended changes to the SAFE manager. An owner who may intend to add a backup SAFE manager which current manager can remove and a current manager could add a backup manager who is not intended to have control. This can lead to security risks and unintended consequences.

Proof of Concept

• Owner Intending to Add 2 SAFE Managers (Desired Behavior):
  • Owner calls allowSAFE(_safe, _SafeManager1, 1) and allowSAFE(_safe, _SafeManager2, 1) to grant permissions to a manager. The desired result is to add managers.
• Current SAFE Manager Adding Unauthorized Backup Manager (Undesired Behavior):
  • Current SAFE manager calls allowSAFE(_safe, _unauthorizedBackupManager, 1) to grant permissions to an unauthorized address.
• Current SAFE Manager(_SafeManager1) removing other SAFE Manager(_SafeManager2) (Undesired Behavior):
  • Current SAFE manager calls allowSAFE(_safe, _SafeManager2, 1) to grant permissions to an unauthorized address.

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

import "forge-std/console.sol";

import {Vault721} from "@contracts/proxies/Vault721.sol";
import {ODSafeManager} from "@contracts/proxies/ODSafeManager.sol";
import {ISAFEEngine} from "@interfaces/ISAFEEngine.sol";
import {Test} from "lib/forge-std/src/Test.sol";
import {DummySAFEEngine} from "test/mocks/SAFEEngineForTest.sol";

contract SafeManagerTest is Test {
    DummySAFEEngine mockSafeEngine;
    ODSafeManager safeManager;
    address public governer;
    address public user1;
    address public user2;
    address public user3;
    // address public randomDestination;
    address public unauthorizedManager;

    address public user1Proxy;
    address public user2Proxy;
    address public user3Proxy;

    uint256 public safeId;

    function setUp() public {
        governer = address(0x0111);
        user1 = address(0x0222);
        user2 = address(0x0333);
        user3 = address(0x0444);
        // randomDestination = address(0x0555);
        unauthorizedManager = address(0x0666);

        mockSafeEngine = new DummySAFEEngine();

        Vault721 vault721 = new Vault721(governer);
        safeManager = new ODSafeManager(
            address(mockSafeEngine),
            address(vault721)
        );

        user1Proxy = vault721.build(user1);
        user2Proxy = vault721.build(user2);
        user3Proxy = vault721.build(user3);

        safeId = safeManager.openSAFE(bytes32("WETH"), address(user1Proxy));
    }

    function test_transferBad() public {
        vm.startPrank(user1Proxy);
        // call this `function allowSAFE(address _usr, uint256 _ok)`
        safeManager.allowSAFE(safeId, address(user2Proxy), 1);
        safeManager.allowSAFE(safeId, address(user3Proxy), 1);
        vm.stopPrank();
        vm.startPrank(user2Proxy);
        // now safeManager.allowSAFE calls `function allowSAFE(uint256 _safe, address _usr, uint256 _ok)` to remove the user2Proxy
        safeManager.allowSAFE(safeId, address(user3Proxy), 0);
        // now safeManage adds unauthorizedManager to the safe
        safeManager.allowSAFE(safeId, address(unauthorizedManager), 1);
    }
}

Tools Used

Manual Review, vscode, Foundry testing

Recommended Mitigation Steps

remove modifier safeAllowed(uint256 _safe) function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external { address _owner = _safeData[_safe].owner;

• require(_owner == msg.sender,"InValid"); safeCan[_owner][_safe][_usr] = _ok; emit AllowSAFE(msg.sender, _safe, _usr, _ok); }

Assessed type

Access Control