search

code-423n4 / 2021-09-sushimiso-findings

0 stars 0 forks source link

Payable external init is redundant and may allow unaccounted token claims or DoS #53

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

There is an external payable init() function in all four auction contracts which has an empty body and the comment seems to indicate that this was later replaced by the initMarket() function but left behind.

This function allows anyone to accidentally/intentionally deposit unaccounted ETH without reverting with revertBecauseUserDidNotProvideAgreement() (as in receive()), and allow them to claim that they should indeed receive auction tokens.

An attacker/griefer could also use this function to deposit ETH by front-running cancelAuction or setAuctionPrice transactions that depend on zero commitments to be successful.

Given the emphasis put on revertBecauseUserDidNotProvideAgreement in commit ETH/token functions and the payable receive function, it appears that this accidental/intentional deposit of unaccounted ETH via init() could be a serious concern from the auction’s perspective — hence classifying this as medium severity.

Proof of Concept

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

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

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

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

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove function or cause it to revertBecauseUserDidNotProvideAgreement(). Or document why this is required.

Clearwood commented 3 years ago

The reason for the init() is to conform with the BentoBox factory which requires a payable init() function to be present on the contract

ghoul-sol commented 2 years ago

Because of this part, I'm keeping it as low risk

An attacker/griefer could also use this function to deposit ETH by front-running cancelAuction or setAuctionPrice transactions that depend on zero commitments to be successful.