TurboRouter: deposit(), mint(), createSafeAndDeposit() and createSafeAndDepositAndBoost() functions do not work

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol

Vulnerability details

Impact

The TurboRouter contract inherits from the ERC4626RouterBase contract. When the user calls the deposit, mint, createSafeAndDeposit and createSafeAndDepositAndBoost functions of the TurboRouter contract, the deposit and mint functions of the ERC4626RouterBase contract are called.

    function deposit(IERC4626 safe, address to, uint256 amount, uint256 minSharesOut) 
        public 
        payable 
        override 
        authenticate(address(safe)) 
        returns (uint256) 
    {
        return super.deposit(safe, to, amount, minSharesOut);
    }
...
    function deposit(
        IERC4626 vault, 
        address to,
        uint256 amount,
        uint256 minSharesOut
    ) public payable virtual override returns (uint256 sharesOut) {
        if ((sharesOut = vault.deposit(amount, to)) < minSharesOut) {
            revert MinAmountError();
        }
    }

The deposit and mint functions of the ERC4626RouterBase contract will call the deposit and mint functions of the TurboSafe contract. The TurboSafe contract inherits from the ERC4626 contract, that is, the deposit and mint functions of the ERC4626 contract will be called.

The deposit and mint functions of the ERC4626 contract will call the safeTransferFrom function. Since the caller is the TurboRouter contract, msg.sender will be the TurboRouter contract. And because the user calls the deposit, mint, createSafeAndDeposit, and createSafeAndDepositAndBoost functions of the TurboRouter contract without transferring tokens to the TurboRouter contract and approving the TurboSafe contract to use the tokens, the call will fail.

    function deposit(uint256 amount, address to) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(amount)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), amount);

        _mint(to, shares);

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

Proof of Concept

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol

Tools Used

None

Recommended Mitigation Steps

In the deposit, mint, createSafeAndDeposit, and createSafeAndDepositAndBoost functions of the TurboRouter contract, add code for the user to transfer tokens and approve the use of tokens in the TurboSafe contract. For example:

TurboRouter.sol

+        IERC20(safe.asset).safeTransferFrom(msg.sender,address(this),amount);
+        IERC20(safe.asset).safeApprove(safe,amount);
          super.deposit(IERC4626(address(safe)), to, amount, minSharesOut);

...

+        IERC20(safe.asset).safeTransferFrom(msg.sender,address(this),amount);
+        IERC20(safe.asset).safeApprove(safe,amount);
          super.mint(safe, to, shares, maxAmountIn);

[Joeysantoro]

Router uses Multicall and PeripheryPayments which can be combined to achieve the desired behaviors

[GalloDaSballo]

@Joeysantoro I don't quite understand your counter argument here for createSafeAnd.... type functions

For Deposit and Mint, yes, you can create the safe, and then multicall the approvals, I agree with your counter, the functions don't need the extra approve calls.

However for the functions that deploy a new safe, am not quite sure where the approval happens, see createSafeAndDeposit below

    function createSafeAndDeposit(ERC20 underlying, address to, uint256 amount, uint256 minSharesOut) external {
        (TurboSafe safe, ) = master.createSafe(underlying);

        super.deposit(IERC4626(address(safe)), to, amount, minSharesOut);

        safe.setOwner(msg.sender);
    }

I believe your counter argument could apply if you were deploying new vaults via Create2 so you could deterministically pre-approve the new safe, however in this case you are deploying a new safe, to an unpredictable address and then calling deposit on it. deposit will safeTransferFrom from the router to the vault and I can't quite see how this call won't fail since the router never gave allowance to the safe

Can you please clarify your counter argument for this specific function?

[Joeysantoro]

I was wrong, this issue is valid

[GalloDaSballo]

Per the sponsor reply, I believe the finding to valid, impact is that the code doesn't work so I believe High Severity to be appropriate.

Mitigation seems to be straightforward

[Joeysantoro]

imo this is not high risk because the router is a periphery contract. Its medium at best from a security perspective, but an important find within the context of the correctness of the code.

To clarify, the issue only exists for createSafe... not deposit or mint for the reason I stated

[GalloDaSballo]

@Joeysantoro I think your perspective is valid and perhaps with more context, I would have indeed rated at a lower severity.

My reasoning at the time was that because the code is broken, the severity should be high. On the other hand, we can also argue that the impact is minimal, as any call to those functions simply reverts, no safes with "wrong allowance" are set, and ultimately the impact is just some wasted gas.

The bug doesn't cause a loss of funds nor bricks the protocol in any meaningful way (because this is just a periphery contract).

I think you're right in your logic, at the time of judging I simply focused on how the code didn't work and thought that was reason to raise the severity