`subscribeValidators` function can be DOS

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xf89cc8a251fd2f452829047e0f4dc0268894b0aafd57c7dee6636e66da2128b7 Severity: medium

Description: Description\ In the subscribeValidators function there is a condition

 require(
            msg.value == subscriptionCollateral * validatorIDArray.length,
            "DappnodeSmoothingPool::subscribeValidator: msg.value does not equal subscription collateral"
        );

In this condition we can see that msg.value must be strictly equals to the product of subscriptionCollateral * validatorIDArray.length. Sending exact msg.value is difficult so if by any chance user send less/more value as little as 1 wei the function would revert and this will keep on happening and he would not know the reason.

so if user would keep on trying but this function will continue to revert causing DOS

Attachments

https://github.com/dappnode/mev-sp-contracts/blob/3929e24ea288d697d38948b8690c8c2028e5042b/contracts/DappnodeSmoothingPool.sol#L255

Revised Code File (Optional)

consider changing the condition to

require(
            msg.value >= subscriptionCollateral * validatorIDArray.length,
        );

In this way if user send more msg.value then the transaction will be proceeded but then make sure to refund the Dust amount of ETH.

[invocamanman]

Making the condition more flexible adds error prone for the user/UI, and makes more difficult the tasks done by the oracle node ( if it has to be distributed later on). The user should use an UI or should be an experienced user, and therefore the multiplication will be done automatically. Also this could be seen this check as a sanity check.

[Nabeel-javaid]

Making the condition more flexible adds error prone for the user/UI, and makes more difficult the tasks done by the oracle node ( if it has to be distributed later on). The user should use an UI or should be an experienced user, and therefore the multiplication will be done automatically. Also this could be seen this check as a sanity check.

there are alot of assumptions in the above comment especially that the The user should use an UI or should be an experienced user so i suggest to have another look at this issue

[Nabeel-javaid]

hey @fonstack idk why you invalidated this, in the above comment you said that the user should use UI and should be experienced use, isn't it a vague assumption, also this is the audit of smart-contract so yes i guess this is a valid med severity issue

[fonstack]

hey @Nabeel-javaid, the committee closed this issue because they think it is invalid. I just reopened the issue and labeled it as invalid. If you disagree with the label given by the project, you can dispute the decision in the dispute phase.

cc: @invocamanman