router: attacker can pull on behalf of users

Title

Attacker can deposit on behalf of users

Affected smart contract

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseRouter.sol#L135-L146

Description

The BaseRouter contract allows bundling multiple actions in one call to execute them in one transaction. For deposit and payback actions, it will pull tokens from sender to this contract.

if (actions[i] == Action.Deposit) {
    // DEPOSIT
    (IVault vault, uint256 amount, address receiver, address sender) =
      abi.decode(args[i], (IVault, uint256, address, address));

    address token = vault.asset();
    _checkBeneficiary(receiver);
    _addTokenToList(token);
    _safePullTokenFrom(token, sender, receiver, amount);
    _safeApprove(token, address(vault), amount);

    vault.deposit(amount, receiver);
  }

Function _safePullTokenFrom is implemented as:

function _safePullTokenFrom(
    address token,
    address sender,
    address owner,
    uint256 amount
  )
    internal
  {
    if (sender != address(this) && (sender == owner || sender == msg.sender)) {
      ERC20(token).safeTransferFrom(sender, address(this), amount);
    }
  }

Then when sender != address(this) and sender == receiver (owner in function_safePullTokenFrom), this contract will pull tokens from sender. It means anyone can deposit on behalf of an user if his/her allowance with this contract still greater than 0.

Attack scenario

Alice approves tokens (example, approve type(uint256).max of USDC) to the router contract, and executes some actions.
After that, Alice's allowance with the router contract still very large, Alice's balance of USDC = 1e6.
Attacker execute a deposit action with amount = 1e6, receiver = Alice's address, sender = Alice. Then 1e6 USDC of Alice will be deposited to vault, and Alice will receive vault shares. => User funds can be deposited unintentionally

Recommendation

Should only allow sender == msg.sender to pull tokens as following:


function _safePullTokenFrom(
    address token,
    address sender,
    address owner,
    uint256 amount
  )
    internal
  {
    if (sender == msg.sender)) {
      ERC20(token).safeTransferFrom(sender, address(this), amount);
    }
  }
``

Fujicracy / fuji-v2