code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol#L114-L133 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L171 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L210 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L258 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L310 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L335

Vulnerability details

Impact

The TurboRouter is not able to interact with an existing TurboSafe because of the authentication modifier of the respective TurboSafe functions. Because of that, those router functions are unusable.

Proof of Concept

Here's the test file I used to confirm it. I had to modify the contracts a little bit to satisfy the dependencies so you can't just drop it into the codebase. But that should be fairly easy for you to replicate.

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity 0.8.10;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {Authority} from "solmate/auth/Auth.sol";
import {DSTestPlus} from "solmate/test/utils/DSTestPlus.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {WETH} from "solmate/tokens/WETH.sol";

import {MockCToken} from "./mocks/MockCToken.sol";
import {MockPriceFeed} from "./mocks/MockPriceFeed.sol";
import {MockFuseAdmin} from "./mocks/MockFuseAdmin.sol";
import {MockComptroller} from "./mocks/MockComptroller.sol";

import {TurboClerk} from "../modules/TurboClerk.sol";
import {TurboBooster} from "../modules/TurboBooster.sol";

import {TurboSafe} from "../TurboSafe.sol";
import {TurboMaster} from "../TurboMaster.sol";
import {TurboRouter} from "../TurboRouter.sol";

import {IERC4626, ERC4626RouterBase, PeripheryPayments} from "ERC4626/ERC4626RouterBase.sol";

contract TurboRouterTest is DSTestPlus {

    MockFuseAdmin fuseAdmin;

    MockComptroller comptroller;

    MockERC20 fei;

    MockERC20 asset;

    MockCToken mockCToken;

    TurboMaster master;
    TurboRouter router;
    User user;

    WETH weth = new WETH();
    function setUp() public {
        fei = new MockERC20("Fei USD", "FEI", 18);

        asset = new MockERC20("Mock Token", "MOCK", 18);

        fuseAdmin = new MockFuseAdmin();

        comptroller = new MockComptroller(address(fuseAdmin), new MockPriceFeed());

        master = new TurboMaster(comptroller, fei, address(this), Authority(address(0)));

        MockCToken assetCToken = new MockCToken(asset);

        comptroller.mapUnderlyingToCToken(asset, assetCToken);

        assertEq(master.getAllSafes().length, 1);

        router = new TurboRouter(master, "router2", weth);
    }

    function testDeposit() public {
      // removed `createSafe()` authentication modifier for this to work
      router.createSafe(asset);
      TurboSafe safe = master.getAllSafes()[1];
      router.sweep(safe, address(this), asset, 1);
    }
}

The test reverts with the following msg: UNAUTHORIZED

Tools Used

none

Recommended Mitigation Steps

Incoming calls from the Router shouldn't be authenticated again since they already are by the Router: https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol#L38

It already checks whether the caller owns the safe. If they do there shouldn't be a reason to block their access. So the authentication modifier of the TurboSafe functions should whitelist the router.

Joeysantoro commented 2 years ago

The auth will be configured appropriately for launch, this is a configuration concern and not an issue with the code

GalloDaSballo commented 2 years ago

I appreciate the warden showing POC for the finding, however had they set up authentication properly, the system would work.

While some discussion about testing auth could be raised (I believe auth is tested in Solmate to provide a baseline guarantee), I don't think there's any vulnerability here.

The fact that the sponsor didn't test the auth through an integration test is not proof that the system will break, because if you change the test to provide auth to the router, your test would no longer revert