YearnTokenAdapter allows a maximum loss of 100% when withdrawing

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L13
https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L43

Vulnerability details

Impact

YearnTokenAdapter allows slippage of 100% when withdrawing from the vault which will cause a loss of funds.

Here's the documentation straight from the vault contract: https://github.com/yearn/yearn-vaults/blob/main/contracts/Vault.vy#L1025-L1073
It allows the user to specify the maxLoss as the last parameter. It determines how many shares can be burned to fulfill the withdrawal. Currently, the contract uses 10.000 which is 100%. Meaning there is no slippage check at all. This is bound to cause a loss of funds.

I'd suggest letting the user determine the slippage check themselves instead of hardcoding it.

Proof of Concept

Current maxLoss value: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L13

call to Yearn vault's withdraw() function: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L43

Tools Used

none

Recommended Mitigation Steps

Allow the user to specify the slippage value themselves:

    function unwrap(uint256 amount, address recipient, uint maxLoss) external override returns (uint256) {
        TokenUtils.safeTransferFrom(token, msg.sender, address(this), amount);

        uint256 balanceBefore = TokenUtils.safeBalanceOf(token, address(this));

        uint256 amountWithdrawn = IYearnVaultV2(token).withdraw(amount, recipient, maxLoss);

        uint256 balanceAfter = TokenUtils.safeBalanceOf(token, address(this));

        // If the Yearn vault did not burn all of the shares then revert. This is critical in mathematical operations
        // performed by the system because the system always expects that all of the tokens were unwrapped. In Yearn,
        // this sometimes does not happen in cases where strategies cannot withdraw all of the requested tokens (an
        // example strategy where this can occur is with Compound and AAVE where funds may not be accessible because
        // they were lent out).
        if (balanceBefore - balanceAfter != amount) {
            revert IllegalState();
        }

        return amountWithdrawn;
    }

If you don't want to change the ITokenAdapter interface you can also leave the value blank. The vault will then use the default value (0.01%)

[0xfoobar]

Sponsor acknowledged

This could be made more configurable by the end user but yearn vaults do not frequently experience high slippage. Slippage is handled upstream in the Alchemist contract. The reason why this slippage is set to 100% is so to permit handling of slippage in the Alchemist for all cases.

[0xleastwood]

Because we can't know how the yearn strategy implements withdrawals, its possible that it might contain custom swap logic which exposes itself to sandwich attacks. However, at face value, the current use of MAXIMUM_SLIPPAGE allows the contract to successfully unwrap their tokens under poor network conditions, but it makes sense for the user to have more control over this. Downgrading this to medium risk as I believe it is more in line with that.