Tokens can be stolen through `transferTo`

[code423n4]

Handle

@cmichelio

Vulnerability details

Vulnerability Details

I know that it's stated that:

VADER, USDV, SYNTHS all employ the transferTo() function, which interrogates for tx.origin and skips approvals. The author does not subscribe to the belief that this is dangerous

In my opinion, it can be very dangerous. Imagine the following scenario:

1. I create a custom attacker ERC20 token that has a hook in the _transfer function that checks tx.origin for USDV/VADER/SYNTHS and calls transferTo to steal these funds.
2. I set up a honeypot by providing liquidity to the BASE <> ATTACKER pool.
3. I target high-profile accounts holdinging VADER/USDV/SYNTHS and airdrop them free tokens.
4. Block explorers / Vader swap websites could show that this token has value and can be traded for actual BASE tokens.
5. User wants to sell the airdropped ATTACKER token to receive valuable tokens through the Vader swap and has all their tokens (that are even completely unrelated to the tokens being swapped) stolen.

Impact

In general, a holder of any of the core assets of the protocol risks all their funds being stolen if they ever interact with an unvetted external contract/token. This could even be completely unrelated to the VADER protocol.

Recommended Mitigation Steps

Remove transferTo and use permit + transferFrom instead to move tokens from tx.origin.

[strictly-scarce]

This attack path has already been assessed as the most likely, no new information is being presented here.

Do not interact with attack contracts, interacting with an ERC20 is an attack contract.

[0xBrian]

@strictly-scarce What would be the downside of adopting the suggested mitigation? Since we cannot communicate effectively with all users to tell them not to interact with certain kinds of contracts (and even if we could, they may not be able to discern which are OK and which aren't), we don't want to set up a thicket for fraudsters to operate. If the downside of the mitigation is not too bad, I think it could be worth it in order to deny fraudsters an opportunity to steal.

[Mervyn853]

Our decision matrix for severity:

0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.

Recommended: 0

[MrToph]

// TransferTo function
// Risks: User can be phished, or tx.origin may be deprecated, optionality should exist in the system.
function transferTo(address recipient, uint256 amount)
    public
    virtual
    override
    returns (bool)
{
    _transfer(tx.origin, recipient, amount);
    return true;
}