Vader Protocol contest - Critical's results

Handle

Critical

Vulnerability details

Impact

Attackers can steal funds from users' balances for those who approved the VaderPoolV2 contract.

Proof of Concept

https://github.com/code-423n4/2021-12-vader/blob/9fb7f206eaff1863aeeb8f997e0f21ea74e78b49/contracts/dex-v2/pool/VaderPoolV2.sol#L165

nativeAsset.safeTransferFrom(from, address(this), nativeDeposit);

VaderPoolV2.mintSynth() allows anyone to transfer funds from an arbitrary address (the from parameter), and mint synth tokens to another arbitrary address (the to parameter).

The current common practice usually ask users to approve an unlimited amount of allowance to contracts. Normally, the allowance can only be used by the user who initiated the transaction.

If that's the case, then the attacker will be able to steal all the wallet balances of the users who approved the VaderPoolV2 contract.

Even if the dApp only asks for the allowance of the amount they plan to mint with, the attacker can still frontrun the user's mintSynth() transaction to initiate the attack.

For instance:

1. Alice approved 40,000 USDV to VaderPoolV2 contract;

2. The attacker called mintSynth() with:

   • foreignAsset = WETH
   • nativeDeposit = 40,000
   • from = Alice's address
   • to = The attacker's address

As a result, 40,000 USDV was transferred from Alice's balance, and the attacker received synth tokens.

3. The attacker called burnSynth() to burn all the synth tokens received.

As a result, the attacker will receive ~40,000 USDV.

Tools Used

Tenderly, Kovan Testnet

The example in Proof of Concept has been tested on a forking network with Tenderly.

Recommended Mitigation Steps

Remove from parameter and always transfer funds from msg.sender.

function mintSynth(
    IERC20 foreignAsset,
    uint256 nativeDeposit,
    address to
)
    external
    override
    nonReentrant
    supportedToken(foreignAsset)
    returns (uint256 amountSynth)
{
    nativeAsset.safeTransferFrom(msg.sender, address(this), nativeDeposit);

    ISynth synth = synthFactory.synths(foreignAsset);

    if (synth == ISynth(_ZERO_ADDRESS))
        synth = synthFactory.createSynth(
            IERC20Extended(address(foreignAsset))
        );

    (uint112 reserveNative, uint112 reserveForeign, ) = getReserves(
        foreignAsset
    ); // gas savings

    amountSynth = VaderMath.calculateSwap(
        nativeDeposit,
        reserveNative,
        reserveForeign
    );

    // TODO: Clarify
    _update(
        foreignAsset,
        reserveNative + nativeDeposit,
        reserveForeign,
        reserveNative,
        reserveForeign
    );

    synth.mint(to, amountSynth);
}

The same issue exists in mintFungible() and shall be fixed similarly:

https://github.com/code-423n4/2021-12-vader/blob/9fb7f206eaff1863aeeb8f997e0f21ea74e78b49/contracts/dex-v2/pool/VaderPoolV2.sol#L329-L330