Missing zero-address check on beneficiary may lead to loss of funds

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

Zero-address checks are a best-practice and are generally low-severity if missing checks lead to reverts. However, the missing zero-address check for beneficiary address (user supplied argument) in commitEth may effectively lead to loss of user funds to the contract and hence is of medium severity. The indication of its importance is illustrated by the fact that this check is present in Crowdsale’s _addCommitment() but missing in Batch, Dutch and Hyperbolic auction contracts.

Proof of Concept

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Auctions/Crowdsale.sol#L309

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Auctions/BatchAuction.sol#L190-L197

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Auctions/BatchAuction.sol#L235-L245

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Auctions/DutchAuction.sol#L261-L282

https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Auctions/HyperbolicAuction.sol#L265-L287

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add zero-address check for beneficiary address.

[Clearwood]

It is a nice practice to safeguard the user but does not have a high severity

[ghoul-sol]

it's a best practice recommendation, non-critical