Lack of Input Validation

[code423n4]

Handle

TimmyToes

Vulnerability details

Impact

Adapter contracts could be deployed in an unusable state. System is more vulnerable to manipulation from extreme inputs, nothing good can come of calling various functions with zero values (wasted gas at least/unexpected behaviours at worst).

Proof of Concept

https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YearnVaultAdapter.sol#L33-L34
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L30-L31
constructor doesn't check for zero address
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YearnVaultAdapter.sol#L63
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L59
deposit() does not check for zero deposit or sanity check token values for possible flash loan attacks.
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YearnVaultAdapter.sol#L72
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L68
withdraw() does not check for zero address as recipient, zero withdrawal amount, or sanity check amounts.
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/Alchemist.sol#L611
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/Alchemist.sol#L575
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/Alchemist.sol#L512
https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/Alchemist.sol#L483
The _amount parameter to all these contracts should be checked for zero value and it may also be beneficial to sanity check against extremely high amounts too.

Recommended Mitigation Steps

Implement the recommended checks with require().

[Xuefeng-Zhu]

What is the attack scenario for zero amount?

For withdraw in YaxisVaultAdapter.sol, since they will only be trigger by Alchemist through AlchemistVault, the recipient will never be zero address

[0xleastwood]

There is no attack scenario, hence marking this as non-critical.