In Lesson 60 of PuppyRaffle, Smart Contract Winners Rejecting Payments could be a High with this Additional Impact

[Kenn-eth]

The withdrawFees function requires this:

require(
            address(this).balance == uint256(totalFees),
            "PuppyRaffle: There are currently players active!"
        );

This means that apart from resetting the raffle, if the winner is a smart contract that rejects payments, then totalFees cannot be withdrawn, disrupting the entire functionality of the smart contract. Does this make this finding a high instead of a medium?

[EngrPips]

If the winner is a smart contract that rejects the payment, that won't make this condition revert

require(
            address(this).balance == uint256(totalFees),
            "PuppyRaffle: There are currently players active!"
        );

I would like more explanation to understand better what you are pointing out.

[theirrationalone]

That winning smart contract would lose his opp. to withdraw and on next spin there could have a different winner. I think IMO your point isn't wrong that much. In some other scenarios your point could be validated. However it's not that one ;`)

[EngrPips]

Interesting.

[theirrationalone]

To clear @Kenn-eth 's doubt it would be enough to say that feeAddress is handled by the owner and whenever the owner sniffs it's a favourable time to withdraw then winner will do so. Moreover, if the selectWinner fails then the whole tx will get reverted.

[Kenn-eth]

Thank you, I got that. But if the selectWinner fails, the contract balance will be totalFees plus prizePool so the condition address(this).balance == uint256(totalFees) still fails. In other words the winner must be paid first before fees can be withdrawn, even if it's fees from a previous raffle. Do you see it now?

I guess this is not a big issue as the selectWinner function can be called again but overall it is not a great design logic.

[EngrPips]

Indeed. not a serious issue.

[theirrationalone]

If selectWinner fails then everything done by the selectWinner so far would get rollBack aka reverted and all states set back to as they were before.

The DoS you're trying to heck here is possible here via selfDestruct

[EngrPips]

interesting