code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The ERC20 spec does not require approve() to revert, so the return value should be checked to determine if it was successful.

Proof of Concept

The following Forge mock and test will allow a Vault to be created/modified even if the call to approve() returns false.

// Docgen-SOLC: 0.8.15
pragma solidity ^0.8.15;

import { ERC20 } from "openzeppelin-contracts/token/ERC20/ERC20.sol";

contract MockNoApprovalERC20 is ERC20 {
  uint8 internal _decimals;

  constructor(
    string memory name_,
    string memory symbol_,
    uint8 decimals_
  ) ERC20(name_, symbol_) {
    _decimals = decimals_;
  }

  function decimals() public view override returns (uint8) {
    return _decimals;
  }

  function mint(address to, uint256 value) public virtual {
    _mint(to, value);
  }

  function burn(address from, uint256 value) public virtual {
    _burn(from, value);
  }

  function approve(address , uint256 ) public virtual override returns (bool) {
    return false;
  }
}

// SPDX-License-Identifier: GPL-3.0
// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import { Test } from "forge-std/Test.sol";
import { MockNoApprovalERC20 } from "../utils/mocks/MockNoApprovalERC20.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";

contract VaultNoApprovalTest is Test {
  using FixedPointMathLib for uint256;

  MockNoApprovalERC20 asset;
  MockERC4626 adapter;
  Vault vault;

  address feeRecipient = address(0x4444);
  address alice = address(0xABCD);
  address bob = address(0xDCBA);

  function setUp() public {
    vm.label(feeRecipient, "feeRecipient");
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    asset = new MockNoApprovalERC20("Mock Token", "TKN", 18);
    adapter = new MockERC4626(IERC20(address(asset)), "Mock Token Vault", "vwTKN");

    address vaultAddress = address(new Vault());
    vm.label(vaultAddress, "vault");

    vault = Vault(vaultAddress);
    vault.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 0, withdrawal: 0, management: 0, performance: 0 }),
      feeRecipient,
      address(this)
    );
  }

  /*//////////////////////////////////////////////////////////////
                          INITIALIZATION
    //////////////////////////////////////////////////////////////*/
  function test__metadata() public {
    address vaultAddress = address(new Vault());
    Vault newVault = Vault(vaultAddress);

    uint256 callTime = block.timestamp;
    newVault.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 100, withdrawal: 100, management: 100, performance: 100 }),
      feeRecipient,
      bob
    );

    // NOTE - vault is initialized even though the approval failed, subsequent tests fail
    // assertEq(asset.allowance(address(newVault), address(adapter)), type(uint256).max);
    assertEq(asset.allowance(address(newVault), address(adapter)), 0);
  }
}

Tools Used

Slither, Forge

Recommended Mitigation Steps

Check the return value for ERC20.approve or use SafeERC20.safeApprove()

Check return value:

error ERC20ApprovalFailed();

if (!asset.approve(address(adapter_), type(uint256).max)) return ERC20ApprovalFailed();

Use SafeERC20.safeApprove()

using SafeERC20 for IERC20;

asset.safeApprove(address(adapter_), type(uint256).max);

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Out of scope

code-423n4 / 2023-01-popcorn-findings