code-423n4 / 2023-07-tapioca-findings

13 stars 9 forks source link

Wrapped native asset doesn't have `safeTransfer` function implemented so it reverts #522

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/YieldBox/blob/master/contracts/YieldBox.sol#L209 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/aave/AaveStrategy.sol#L273 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/balancer/BalancerStrategy.sol#L212 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/compound/CompoundStrategy.sol#L159 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/convex/ConvexTricryptoStrategy.sol#L344 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/curve/TricryptoNativeStrategy.sol#L239 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/glp/GlpStrategy.sol#L124 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/lido/LidoEthStrategy.sol#L164 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/stargate/StargateStrategy.sol#L266 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/yearn/YearnStrategy.sol#L147

Vulnerability details

Impact

User funds can't be withdrawn

Description

In every Yieldbox strategy contract the _withdraw() logic uses wrappedNative.safeTransfer(to, amount);. Yieldbox strategies are assumed to deployed to mainnet assuming from the fork test environment variables. WETH on Ethereum doesn't have safeTransfer() function implemented.

In Yieldbox strategy contracts wrappedNative or weth in GlpStrategy.sol is initialized as an IERC20 with the BoringERC20 library via using BoringERC20 for IERC20; declared at the start of the contracts. BoringERC20 assumes safeTransfer() is an implemented function of the passed in address assumed to be a wrapped native asset, however it's not the case.

Futhermore since Tapioca launches on on also Arbitrum, Optimism and later planned to launch on Fantom and Avalanche, if any potentially new strategy contract that's written specific to these chains would use safeTransfer on the native asset, it would revert as well.

WETH source code on Ethereum, Arbitrum, Optimism, WAVAX source code on Avalanche WFTM source code on Fantom

This vulnerability is also present in YieldBox.sol - depositETHAsset()

Proof of Concept

Foundry mainnet fork test repository for the PoC:

  1. forge init tapioca-weth-reverts && cd tapioca-weth-reverts
  2. install dependency: - forge install boringcrypto/BoringSolidity
  3. create test file: touch test/WethReverts.t.sol and copy & paste the Proof of Concept:
    
    // SPDX-License-Identifier: UNLICENSED
    pragma solidity ^0.8.13;

import "forge-std/Test.sol"; import "../lib/BoringSolidity/contracts/libraries/BoringERC20.sol";

contract WethReverts is Test { using BoringERC20 for IERC20;

uint256 mainnetFork;
string ETH_MAINNET_ALCHEMY_KEY = "https://eth-mainnet.g.alchemy.com/v2/vPlgiIOYhaqzkhCstL1lSu_8e1u0Avzr";

address user = vm.addr(888);
address user2 = vm.addr(99);

address constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
IERC20 weth;

function setUp() public {
    mainnetFork = vm.createSelectFork(ETH_MAINNET_ALCHEMY_KEY);
    weth = IERC20(WETH);
}

function testWethReverts() public {
    deal(address(weth), user, 1e18);
    assertEq(weth.balanceOf(user), 1e18);

    // @note reverts because WETH doesn't have safeTransfer
    // @note and uses the incorrect BoringCrypto IERC20 interface
    weth.safeTransfer(user2, 1e18);
    assertEq(weth.balanceOf(user2), 0);
}

}


4. add your own Alchemy key to `string ETH_MAINNET_ALCHEMY_KEY`
5. run `forge test --match-test testWethReverts -vvvv`
6. keep in mint the test is supposed to fail with `"BoringERC20: Transfer failed"`

## Tools Used
Manual review, Etherscan, Foundry Fork tests

## Recommended Mitigation Steps
Don't use any `IERC20` interface for `wrappedNative`, use WETH's interface instead. Use `transfer()` instead of `safeTransfer()` on `wrappedNative` calls. 

## Assessed type

Error
code423n4 commented 1 year ago

Withdrawn by 0xfuje