Platform: Code4rena
Start Date: 17/02/2022
Pot Size: $75,000 USDC
Total HM: 7
Participants: 23
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 2
Id: 92
League: ETH
Rank: 5/23
Findings: 1
Award: $6,099.18
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: cccz
Also found by: CertoraInc, WatchPug
6099.1815 USDC - $6,099.18
https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol
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); }
https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol
None
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);
#0 - Joeysantoro
2022-02-24T19:10:23Z
Router uses Multicall and PeripheryPayments which can be combined to achieve the desired behaviors
#1 - GalloDaSballo
2022-03-12T16:40:15Z
@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?
#2 - Joeysantoro
2022-03-12T17:40:45Z
I was wrong, this issue is valid
#3 - GalloDaSballo
2022-03-13T17:05:50Z
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
#4 - Joeysantoro
2022-04-13T21:57:18Z
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
#5 - GalloDaSballo
2022-04-15T13:04:07Z
@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