My Coding Journey: From Finance to Smart Contract Auditor

Security
,
48

Not true if msg.sender is a contract which does not implement a payable receive function or a payable fallback function.
In other words, msg.sender is a "non-payable" contract which implements a function which executes new King(), and someone has executed that function.

That's wrong.
First of all, they are not "a function that can receive ETH", they are addresses.
Second, the constructor merely stores these input addresses into the contract state variables.
Third, there isn't any straightforward, clean, simple and cheap way to check if an address can receive ETH. At least not onchain (i.e., in a contract code). Doing it offchain (e.g., in web3.js script) is pretty simple, and of course - doesn't cost any gas.

FYI, you may as well just declare address public king (like you do with each one of the other two state variables).
The compiler will then implicitly generate a public function of the same name (king), which returns the value of that variable.

Well, you haven't quite stated up until this point what the problem is, so jumping to "The Solution" here is a bit confusing. But anyway, here are the problems that I see in this contract:

Problem #1 - Financial Incentive

First of all, every time an account which is not the owner transfers an amount of ETH to your contract:

  • If that amount is smaller than the current value of prize, then the transaction reverts, and the caller has spent gas for nothing
  • If that amount is equal to or larger than the current value of prize, then the caller gets it back and ends up spending gas for nothing

The latter bullet applies for the owner account regardless of the amount of ETH.
So in short, there is absolutely no incentive for anyone to ever use your contract.

Problem #2 - Receiving ETH

There are two potential issues in your contract's receive function:

  1. It might be subjected to reentrancy
  2. It might revert due to gas stipend

The account sending ETH to your contract can be any one of 5 different types.
The account pointed by address king can be any one of 5 different types.

Let's denote these types:

  1. EOA
  2. SCA which does not implement a receive or fallback function
  3. SCA which implements a receive or fallback function which does not send ETH back to msg.sender
  4. SCA which implements a receive or fallback function which sends ETH back to msg.sender via send or transfer
  5. SCA which implements a receive or fallback function which sends ETH back to msg.sender via call

So all in all, you have 25 different scenarios, under which you need to inspect each one of the 2 potential issues above.
I'm obviously not going to list all of them, but here are a couple of points which you'd need to take into account:

Point 1

When contract X receives ETH from a function in contract Y via transfer or send executed in that function, the function in contract X (receive or fallback) is limited by a stipend of 2300 gas.
In your contract, that function updates two state variables (king and prize) which are located in two separate storage slots.
And updating even a single storage slot is likely to exceed that gas stipend.
At the time of writing this:

  • Around 2100 gas if the variable's value doesn't change
  • Around 5000 gas if the variable's value changes from non-zero to non-zero
  • Around 22100 gas if the variable's value changes from zero to non-zero

Hence you may be facing a bunch of scenarios in which an attempt to send ETH to your contract would revert due to gas stipend (issue #2 above).
This is not to say that there's an actual problem here.
In fact, it might even be desirable in the sense that it prevents reentrancy attacks (issue #1 above).
But you'd still need to look into this under the perspective of product requirements.
In other words, you'd need to ask yourself if there's a product requirement for supporting a specific scenario, which is not handled as desired.

Point 2

When contract X receives ETH from a function in contract Y via call executed in that function, the function in contract X (receive or fallback) is NOT limited by any gas stipend.
This may open your contract's receive function to reentrancy attacks.
In your specific case, it is not really clear what could possibly be achieved by such an attack, other than the attacker spending their own ETH.
But you should nevertheless look into this under the perspective of security vulnerabilities.

1 Like