code423n4 commented 2 years ago

Handle

leastwood

Vulnerability details

Impact

The constructor() and several other functions do not validate input data. As a result, users and/or privileged roles may mistakenly call the respective functions with improper inputs, leading to unintended consequences and incorrect contract states.

Proof of Concept

https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L51-L58 https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L73-L76 https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L106-L114

Tools Used

Manual code review

Recommended Mitigation Steps

Consider checking for the zero address if the input variable is of type address. Additionally, swapFee in the constructor() should be checked to ensure it is strictly less than SWAP_FEE_DIVISOR.

Shadowfiend commented 2 years ago

Consider checking for the zero address if the input variable is of type address.

feeRecipient is intentionally 0-settable (see the NatSpec). Most other addresses in the linked sections are checked for 0-ness, except for zrxTo; will double-check if there's more validation we can do there.

Additionally, swapFee in the constructor() should be checked to ensure it is strictly less than SWAP_FEE_DIVISOR.

Great catch!