Beware of bad voodoo when talking to strangers.
protocol assures us that transactions are atomic. That is, they either succeed completely or they don’t do anything (important) at all. Sounds great, right?
One of the more challenging aspects of adapting to this environment is internalizing non-obvious risks. We need to learn about new kinds of danger and form habits that will help us avoid trouble.
This post will reveal various ways that naive assumptions can get a contract into trouble, show you how vulnerable contracts can be interfered with. The author will suggest some habits you can form to protect yourself.
Let us consider a naive contract. Let’s just consider a situation where funds need to be distributed to more than one account. It could be an air drop, a dividend or something else. Let’s say everyone has contributed to a crowd funding project but the funding goal wasn’t reached and now everyone should get their money back.
The Naive Way
function sendRefunds() public onlyOwner returns(bool success)
return true;
}
Great. For every funder, return the contribution (Let’s just say the amount of the contribution was the same for everyone). Only the owner is allowed to make that happen.
I set most application details aside so we can focus on what is wrong with this approach to a seemingly simple task.
Serious Problems
The serious problems aren’t obvious.
On a deep design level, I would argue that the contract doesn’t assure anyone they will get their money back. Only the owner can initiate the function, but they are distraught, possibly in hiding, possibly irresponsible, possibly have lost the only signing key that can make it happen. The owner is a single point of failure and this pattern suggests adherence to server-centric thinking — only a privileged user should be able to initiate a mass distribution of funds.
Such thinking misses the point of a distributed system and the assurances that are possible with a smart contract. If the people are entitled to a refund, then there should be no possibility of interference in that process.
Another error is the unbounded for loop. It will surely run out of gas at some maximum number of funders making it impossible for anyone to recover the funds. That is not good. It’s an anti-pattern. If the foregoing claim sounds mysterious or poorly-explained, see .
A more subtle problem
Suppose Alice and Bob are the two funders and for some reason the campaign is limited to two funders. Further suppose that the developer is a purist and wants to avoid using a loop. The function could be re-imagined in a more idiomatic fashion:
function sendRefunds() public onlyOwner returns(bool success)
- What could go wrong, right? There is no conditional logic. We know transactions are atomic. There are three simple steps to complete. Can you you imagine a scenario where no one gets paid (other the owners not playing ball as mentioned)? Take your time. The next bullet is a spoiler.
- What would happen if Bob rejected the transfer? Think about it. Take your time.
- Would Alice get paid? Would the transaction succeed or fail? Take your time.
Spoiler alert
If Bob rejects the transfer, the transfer method reverts and this causes the entire transaction to fail, meaning the sender (signer, owner) gets an error. The transaction is atomic. It’s an all-or-nothing proposition, right? So, if any participant rejects the transfer then no participant gets paid. Said another way, if one fails then they all fail.
Why would that happen? An intuitive way to look at this is to assume that all accounts want and accept all funds at all times because receiving money is always desirable. That’s naive. That would be making assumptions about what the participants find desirable. If they find it more desirable to interfere with the refund process and the contract allows it, then we can expect that to happen.
How?
Fallback functions are not payable
Contracts have fallback functions that run when such transfers are received. By default they look roughly like:
function () public
Notably, the default fallback is not payable, so it will reject all funds. It’s good practice for contracts to reject all unexpected funds. Contracts usually have accounting needs to attend to and they don’t want these “out of the blue” transfers. By default, a contract will reject the transfer. This is deadly for the naive contract if it happens to accept funds from a contract. It can’t work unless everyone cooperates. By extension, it can’t work if even one funder is a contract with default setup.
If trouble can happen by accident, trouble can happen on purpose
The possibility of an accident also means there is a possibility of a denial-of-service attack. An attacker might send a small contribution to a fund-raiser, take its place among the funders and DoS the refund process. That’s trivially simple (and possibly very cheap) sabotage. An attacker could tie up considerable funds and cause reputational damage to the project team. The situation probably wouldn’t be repairable. Indeed, a determined attacker could jam up the refund, effectively holding the funds hostage, possibly for ransom. Yikes!
Maybe we should disallow contracts
I feel some questionable ideas are worth mentioning. One might look at this and think “Ah, Ha! All I need to do is prohibit contracts from participating.” In my view this idea defeats the idea of reliable modular code that interacts to form more complex systems. Why should you want your contract to discriminate on this basis? Is there no valid case where the client (msg.sender) is a contract? What about autonomous organizations?
One might see this looks promising:
Alas, there is a catch. If an attacker calls the contract from a constructor this method will return false (not a contract) because the caller has no bytecode, yet. It has no bytecode yet because the contructor hasn’t reached that step, yet. Disclosure: this wasn’t my observation but the post I saw about this eludes me at the time of writing. Perhaps a kind soul will chime in so the credit goes where it’s due. The idea is simple enough:
pragma solidity 0.4.25;
contract Victim
return (size > 0);
}
}
contract Attacker
}
You can play around with that in Remix.
- Deploy a Victim
- Deploy an Attacker with the Victim address
- Check
iTrickedIt - True!
It was fooled into thinking the Attacker is not a contract. So, rather than preventing interactions with contracts, this effort to avoid contracts simply instructs attackers to launch their attacks from constructors. Perhaps even worse, attackers will know we are concerned about interacting with contracts, and that transmits a strong hint about where to look for a vulnerability.
Beware of too much voodoo.
Maybe we should check the send
One could dispense with the transfer method and use send instead. It returns a false instead of revert in the case that the value transfer fails. You get something like this:
function sendRefund() public onlyOwner returns(bool success)
}
What exactly are we supposed to do? If Alice won’t accept the funds, then we have some messy accounting to deal with. If we are interested in thoroughness and fairness, then we have to give Alice a way to withdraw those funds.
Withdrawal function
We can make a function for funders to come and claim their funds. This works equally well for externally owned accounts and contracts. It has the added benefit that the beneficiary of the transaction pays for the gas.
Our job is is to know how much money is owed. Let’s set some details aside for a moment and just look at the basic idea. In this context, msg.sender is the account asking for their funds. We will check if the request is acceptable and then proceed.
function claimRefund() public
Never talk to more than one untrusted contract at a time
Notice that the claimRefund() function snippet is only concerned with the msg.sender, that is, who is asking for money right now? Is the request acceptable? If so, do it. It doesn’t rely on the owner doing what the funders expect them to do. It doesn’t rely on the other parties not interfering.
That is a good safety habit. By compartmentalizing interactions with one untrusted party at a time, we make it very hard for users to interfere with others. This approach resolves the central issues we discovered in the naive contract.
Unlimited interactions with trusted contracts. One untrusted contract at a time
You can create vast systems of trusted contracts that talk to each other, and that’s okay. In this context trusted means there is no doubt about its intentions, what it does or how it works. msg.sender could be anyone or anything, so we don’t trust it. One of those at a time. That’s our budget.
We can’t safely open another channel of communication with another “untrusted” contract within a single transaction. While it may be difficult to reason about precisely how a potential attack might unfold, talking to more than one untrusted contract at a time almost always creates an opportunity to interfere.
Withdrawal Pattern
Astute readers will have noticed that the subject of msg.sender’s balance was rather glossed over. How does that work in practice?
The first order of business is to create a data structure that will hold the balance for every possible user. Something like:
mapping(address => uint) public balances;
Great. As the funds arrive:
balances[msg.sender] += msg.value;As the funds are returned:
balances[msg.sender] -= amountToSend;
Of course, you have to actually transfer funds and you have to make it re-entrance safe, so we will adjust the balance before we do the transfer. Altogether, something like:
function withdrawFunds(uint amount) public returns(bool success)
Recap
It’s not safe to talk to more than one untrusted contract at a time. The withdrawal pattern nicely avoids the necessity of doing so. Accounting will vary on a case-by-case basis. Suffice it to say that contracts should be aware of their liabilities to each account.
If we’ve done the accounting, which we should, and if we’ve given everyone a way to claim their funds, which we should, then there is often no need for the naive approach that tries to push transfers to other accounts. In essence, that is what developers mean when they say we should use a “withdrawal pattern” or favoring “pull” over “push”.
Internalizing and practicing this approach habitually is an important aspect of the safe handling of funds in Solidity. As a reminder, production code candidates should always endure rigorous peer review. Problems might not be obvious. The withdrawal pattern is one way you can eliminate obvious vulnerabilities.
At , we are dedicated to guiding students to the top of this field. Our hands-on mentored by instructors like myself and workshops prepare students for stringent certification exams.
Start with your and consider branching out to (think “enterprise”) or (think “smart contract auditor”). Maybe even check out. They too are interesting and different.
Published at Tue, 26 Mar 2019 18:09:19 +0000
