Fake balances can be add for not-yet-existing ERC20 tokens, which allows attackers to set traps to steal funds from future users

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L99
https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L18

Vulnerability details

Impact

When add a new vault, solmate's SafeTransferLib is used for pulling vault.token from the caller's account, this issue won't exist if OpenZeppelin's SafeERC20 is used instead.

That's because there is a subtle difference between the implementation of solmate's SafeTransferLib and OZ's SafeERC20:

OZ's SafeERC20 checks if the token is a contract or not, solmate's SafeTransferLib does not.

See: https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

As a result, when the token's address has no code, the transaction will just succeed with no error.

This attack vector was made well-known by the qBridge hack back in Jan 2022.

For our project, this alone still won't be a problem, a vault created and wrongfully accounted for a certain amount of balance for a non-existing token won't be much of a problem, there will be no fund loss as long as the token stays that way (being non-existing).

However, it's becoming popular for protocols to deploy their token across multiple networks and when they do so, a common practice is to deploy the token contract from the same deployer address and with the same nonce so that the token address can be the same for all the networks.

For example: $1INCH is using the same token address for both Ethereum and BSC; Gelato's $GEL token is using the same token address for Ethereum, Fantom and Polygon.

A sophisticated attacker can exploit it by taking advantage of that and setting traps on multiple potential tokens to steal from the future users that deposits with such tokens.

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L99

          ````
               function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
    public
    payable
    returns (uint256 lpTokenAmount)
{
    // *** Checks *** //

    // check the token amount inputs are not zero
    require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

    // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
    require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

    // calculate the lp token shares to mint
    lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);

    // check that the amount of lp tokens outputted is greater than the min amount
    require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

    // *** Effects *** //

    // transfer fractional tokens in
    _transferFrom(msg.sender, address(this), fractionalTokenAmount);

    // *** Interactions *** //

    // mint lp tokens to sender
    lpToken.mint(msg.sender, lpTokenAmount);

    // transfer base tokens in if the base token is not ETH
    if (baseToken != address(0)) {
        // transfer base tokens in
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
    }

    emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
}
          ````

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L18

         'using SafeTransferLib for ERC20;'

PoC

Given:

- ProjectA has TokenA on another network;
- ProjectB has TokenB on another network;
- ProjectC has TokenC on another network;

1. The attacker add() for TokenA, TokenB, and TokenC with 10000e18 as tokenIdOrAmount each;
2. A few months later, ProjectB lunched TokenB on the local network at the same address;
3. Alice created a vault with 11000e18 TokenB;
4. The attacker called initiateWithdraw() and then withdraw() to receive 10000e18 TokenB.

In summary, one of the traps set by the attacker was activated by the deployment of TokenB and Alice was the victim. As a result, 10000e18 TokenB was stolen by the attacker.

Recommended Mitigation Steps

Consider using OZ's SafeERC20 instead.

[c4-judge]

berndartmueller marked the issue as duplicate of #245

[berndartmueller]

Downgrading to QA (Low). Please see #245 (comment) for my reasoning.

[c4-judge]

berndartmueller marked the issue as not a duplicate

[c4-judge]

Duplicate of #98