Fractional v2 contest - cloudjunky's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L65

Vulnerability details

Impact

L65 in BaseVault.sol transfers ERC20 tokens using .transferFrom() and doesn’t verify that the transfer is successful. Some tokens like ZRX don’t revert on failure but rather return false. If the BaseVault.sol is used to create a vault the attacker can specify ERC20 tokens they don’t own as collateral.

Proof of Concept

1. An attacker creates a vault using BaseVault.sol and specifies an array of ERC20 tokens and amounts that they don’t own/possess.
2. batchDepositERC20() attempts to transfer the tokens and the ERC20 contract (e.g. ZRX) returns false and this is not checked. The vault is created as if the ERC20 tokens were transferred.
3. Other users may buy into the vault because of the attractive ERC20 balance (that doesn’t exist).
4. If the attacker holds more than 51% of the vault they could initiate a buyout and remove liquidity of other users.

Tools Used

Vim

Recommended Remediation Steps

ERC20 tokens should be transferred using the SafeERC20 library from OpenZeppelin which includes the .safeTransferFrom() function. This would ensure that the ERC token transfer reverts in conditions where the attacker doesn’t possess the ERC20 tokens (i.e. when the ERC20 contract returns false).

To achieve this you could delete the line importing IERC20;

import {IERC20} from "../../interfaces/IERC20.sol";

And then import SafeERC20 from the OpenZeppelin contracts and use it for IERC20’s;

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
using SafeERC20 for IERC20;

Lastly change .transferFrom() to .safeTransferFrom();

IERC20(_tokens[i]).safeTransferFrom(_from, _to, _amounts[i]);

Then you can run all tests with forge test and they pass.