B012: Security Tidbits v3
Continuing the security snippet trend, I am going to investigate some other prime examples of issues that can be faced within smart contract code both small and big that need to be properly vetted and pointed out during a due diligence review.
This time the focus will be ERC-20 contract interactions and how innocuous integrations can lead to devastating exploits or simply denial-of-service attacks taking place.
For the latter, we will deviate a bit from the traditional “denial-of-service” notion and refer to it in terms of smart contracts whereby a state change has caused certain functions of the contract to become in-executable unless otherwise resolved.
Approval Nightmare
A well-known issue with integrating EIP-20 tokens in complex DeFi systems is the way the EIP-20 standard operates. As there is no single transfer-and-call action (although it exists as an ERC-677), users must either pre-approve a contract for a particular expenditure or code their own custom ways the token interacts with their ecosystem.
As the latter is usually not an option for “open” DeFi systems, the approve concept must be utilized by the system’s contracts. Here, we face another, inherent “vulnerability” in the way blockchain works. The approve function was originally designed to simply assign the newly set approval at the storage slot approvals are generally stored for a particular approver-spender pair.
Given that actions in a blockchain are asynchronous, it is possible for an approver to have already set an allowance to a particular spender to say 20 units and then desire to adjust it to 10 units.
Once the approver broadcasts their 10 unit allowance transaction, the spender can detect that and submit their own spending transaction with a higher gas fee to be included first resulting in what is known as a “race-condition”.
If the spender succeeds, they will be able to spend the full 20 units originally approved as well as the newly approved 10 units as the approve function overwrites whatever used to exist as approval instead of incrementing or decrementing it. As a workaround to this issue, developers devised the increaseApproval and decreaseApproval functions that do exactly what their name implies.
This was still deemed an unsatisfactory solution, however, as the standard directly utilizes and urges developers to integrate with the approve function. As a further, more “non-compliant” adjustment to the EIP-20 standard developers began enforcing a simple logic check within approve that ensures non-zero values are set when the storage has already been zeroed out and vice versa.
This ensures that a “race-condition” never arises given that in the above scenario the approver would first zero out the 20 unit approval and would detect if the spender did anything fishy before re-setting it to 10 units.
Although it works wonders for off-chain integration, on-chain contracts need to now be aware of this particular logic state and conduct proper adjustments in the allowance they provide. For example, a smart contract providing liquidity to a Uniswap V2 pool will need to properly account for “unconsumed” approval when providing liquidity multiple times in the same pair.
In general, when a smart contract approves another address it should validate that the allowance is zero already and if not, manually zero it out to ensure that it can be set accordingly. For the actual actions, it may utilize increaseApproval or decreaseApproval, however, the OpenZeppelin SafeERC20 implementation of those functions should be avoided as it is deprecated.
EIP-20 Transfer Validation
The EIP-20 standard denotes that the smart contract implementations of the standard must return a bool variable indicating whether the execution of a transfer or transferFrom invocation has succeeded. This particular trait was introduced to allow smart contracts to code graceful error handling rather than completely halt execution.
A drawback of this particular bit is that many developers are unaware of the standard and can be observed performing raw, unchecked transfer and transferFrom invocations that can lead to misbehaviors, such as a staking implementation not acquiring any funds for a particular stake.
Furthermore, many tokens are not “true” to the standard and simply do not return any bool variable at all. This leads to code that strictly validates the returned value to fail fatally halting execution.
To amend the issue, it is strongly recommended that a low-level call is performed for the transfer / transferFrom invocation that opportunistically validates the returned values by first checking that they exist (the returned byte payload has a non-zero length) and if so, the decoding of the returned value as a bool is true.
This is taken care of by the OpenZeppelin SafeERC20 library which exposes the safeTransfer and safeTransferFrom functions and I usually recommend its utilization unless the OpenZeppelin dependency does not exist at all in the project I am auditing.
Conclusion
Sometimes, over-engineered solutions to either protect or enhance the usability of software can cause inconsistency in the final products that are developed around the standard, even more so in an open ecosystem such as Ethereum.
These issues need to be thought of and caught during a standard’s RFC stage rather than post-publishing, as the latter will lead to a de facto deviation among its implementations. In the end, the only choice for developers is to simply keep up to speed with these advancements and ensure their code is as future-proof as possible.
