A Recap of ChainSecurity’s Smart Contract Audit

TLDR: The audits ChainSecurity conducted on STK’s smart contracts are now done.

First off, we’d like to thank ChainSecurity for auditing our set of smart contracts! Their feedback was incredibly insightful. They were always open to questions about implementations and were helpful in suggesting alternative solutions. They helped a lot with security features of our smart contracts, and with helping us optimize our code.

You can check out the public report of our contracts here:

https://github.com/ChainSecurity/audits/blob/master/ChainSecurity_STK.pdf

While I’m not going to bore you with every detail of the audit that was fixed — like compiler warnings or keeping Solidity versions up to date, I will talk about the larger, more pressing issues that were fixed, like the Unrestricted Ether Flow (Critical) bug and the design-issue causing STACK can steal all ETH (High).

Just for clarity sake — in this blog post, I am discussing the smart contract audit results of our Multitoken Smart Contract repository from the most recent commit at the time of writing: b98def2.

Unrestricted Ether Flow [Critical]

Description of the Bug: There were no checks implemented before calling the transfer function. Thus, anyone was allowed to call it and send ETH arbitrarily between different addresses.

Fix: This bug was a rather simple fix to make. It was a matter of adding a require statement to ensure that the msg.sender == src. This ensured that users are only allowed to transfer a maximum amount of what’s in their account.

STACK can steal all ETH [High]

Background: In a few of our past articles, I talked about the implementation of our Multitoken Payment Channel and Automatic Contract Wrapping of ETH. Our Multitoken functionality allows us to dynamically add tokens, allowing our contracts to be flexible to changing versions. Despite ETH being structurally different, WETH allows us to treat ETH as if it were an ERC20 token.

Description of the Bug: Inherent in the Multitoken functionality of our state channels, it allows STK to dynamically add channels. For instance, to support DAI and STK, we would add those specific channel addresses. To facilitate ETH, the address of the WETH channel would have to be added by STK, which would enable transactions with ETH.

Under the hood, when a user sends ETH into the Multitoken contract, it is stored in said contract. At this point, it is still in ETH form. Either STACK or the user has to call MultitokenChannel.deposit() to convert the ETH into Wrapped ETH. This function will take the entirety of the ETH balance in the Multitoken channel and transfer it to the WETH contract. At this point, the ETH is stored in the WETH contract, and the contract address is assigned a WETH token balance. At the settling of the payment channel, when STK wants to claim the IOU (or when the user requests a return of their tokens), the WETH contract will decrease the token balance, and transfer ETH to a specified address.

However, in the step above, the transfer of funds from the contract address to the WETH address did not require validation from the user. This would allow STK to transfer funds to any address — even if it wasn’t one that we whitelisted. Worse, if STK’s private key was infiltrated, this functionality would allow an attacker to siphon out all ETH from every channel.

Fix: This bug was extremely difficult to find a solution, due to the nature of the issue. As it was a trust issue, it was more of a design-problem than a particular line of code in the contracts. Thus, we devised a few solutions that might help fix it:

1. Every channel is assigned to a single Wrapped ETH channel which can never change. Each user gets a channel — this option would mean each channel is mapped to a single WETH address. If this ever had to change, STK would have to redeploy a new channel for each user. This ultimately defeats the purpose of the Multitoken State Channel functionality, which allows for the ability to update and add tokens as required.
2. Only a user can call deposit(), so a user will always be able to validate what address their funds are going to. While this solution might have worked, it wouldn’t have offered a uniform user experience between ERC20 and ETH, which was the purpose of the Wrapped ETH functionality. If only a user can call the function, users would have to use MetaMask to call a specific function to send funds to it. This is a completely different user experience than what exists for ERC20 tokens, as users just need to send it to the contract address.
3. Remove automated contract wrapping and build a UI for the user to wrap and unwrap ETH themselves. The fundamental goal of the Automated Contract Wrapping and Unwrapping of ETH was to maintain the same user flow. If we removed the automated wrapping, a user would have to convert ETH to WETH using their own Ethereum address. Then, they would have to transfer the WETH into their Multitoken contract. This is unnecessarily complicated.
4. Leave the overall design as-is, which provides usability. This would allow the Multitoken State Channel to maintain the dynamic tokens. However, users would have to trust STK in the transfer and proper deposit of funds. If STK’s private key had been exposed at any time, this would allow attackers to siphon ETH out of all the user’s channel.
5. User and STACK calls deposit() with a user-signed signature after depositing ETH into the Multitoken channel. In order to call deposit(), the smart contract requires a signature signed by the user, validating their affirmation to the WETH address. This would ensure that STK only changes the state with proper authorization from the user. However, this transaction was signed after the user had sent their ETH into the channel, and before the Multitoken channel sends it to the WETH contract. The signature was signed too late. In a real world example, it would be similar to going skydiving, and paying before you signed the waiver. You suddenly read something in the waiver that makes you uncomfortable — or, you just get cold feet. In the fiat world, you request a refund, which will be processed relatively quickly. In our smart contract, there is no way to withdraw the ETH that was sent, without using the WETH channel. In other words, this signature was signed too late.
6. User or STACK calls deposit() with a user-signed signature before depositing ETH. In comparison to Option #5, the alternative is to get the user to sign the transaction before depositing ETH into the channel. Once we push our WETH address onto mainnet, we will make sure to let users know the address. On the mobile UI side, the user will have to agree to using the address before the channel is even added. This signature will be pushed on chain when calling deposit(), which can verify the transaction is authorized by the user. The signature, validating the deposit of funds into that specific WETH address, gives STK the required approval to send ETH into that address. This signature will work for the period of time we are using that WETH version. If the WETH contract is ever updated, we will ask the user to sign on a new channel address. We will continue using this signature until there is an update to the WETH contract, which the user will have to sign again.

Understandably, we chose to go with Option #6.

• Allows us to make use of the flexibility and upgradeability in our Multitoken Payment Channel.
• Preserves a relatively uniform user interface between the loading of ERC20 Tokens (STK, DAI) and ETH through our automated wrapping and unwrapping.
• Signed transaction offers validation that a user has agreed to deposit ETH into a specific channel. Thus, STK has no way to deposit funds into an unauthorized address. STK is also not allowed to change the state without agreement from the user.
• Maintains the decentralization of a state channel, as parties have to agree to state changes.

The above solution is directly related to the medium-level security issue of Users may unintentionally lose ETH, by specifying the wrong channel address to the deposit() function. Using Option #6 means users must validate the address, which prevents unintentional transfer to unknown addresses.

Locked Tokens inside WETH Contract [Low]

Description of Design Issue: Inherent in most things blockchain, you can send your ETH or ERC20 tokens to addresses which don’t support the currency — then your crypto will be locked up forever. Please check out the Returning Lost Tokens section of our Multitoken Blog Post if you do not fully understand the concept of locked tokens.

Our WETH contract implements functionality to support ETH. However, there are no implementations for ERC20 — thus if a user accidentally transfers an ERC20 token to the WETH contract, it is locked and lost forever.

Fix: While we realize that there is a potential for a user to send ERC20 tokens into the WETH contract, the nature of automated unwrapping and wrapping of ETH means transfers are completely handled by the Multitoken contract.

With the current implementation of WETH, there is no reason for a user to directly call functions on a WETH Contract. The user would interact directly with the Multitoken contract, which will propel the ETH into the WETH contract address. The purpose of the automated wrapping and unwrapping is to abstract the user from this secondary contract — so they should not be interacting with the intermediate WETH contract.

Thus, we chose not to implement the rescue of locked tokens in the Wrapped ETH contract.

Multiple Refactors for the WETH Contract

One of the most prevalent differences in transfer in our WETH contract, is they do not meet ERC20 standards.

Our transfer function does not exchange WETH between different addresses. The inputs to this function are sourceAddress, destinationAddress, amount. After double checking that the sourceAddress==msg.sender, the contract will:

1. Decrease the Wrapped ETH balance on sourceAddress by amount
2. Transfer amount in ETH to destinationAddress

Our code at the Multitoken Payment Channel on GitHub has been changed to reflect the implementation difference between typical ERC20 tokens.

Another difference is the removal of the transferFrom function. In a typically ERC20 token, this functionality is transferring tokens from one address to another. However, transferring WETH between users would not be useful for our set of smart contracts. The purpose of the WETH contract is to accept ETH, and make it into something that confirms to ERC20 standards. At withdrawal, the contract would decrease the WETH balance of the sender, and transfer ETH to the destination address.

It’s a wrap!

Good old Admiral Ackbar! Thus, it’s also a wrap on our recap of the Security Audit. If any of the descriptions confuse you, or you have any other questions from the audit that you’re interested in, feel free to comment below or make an issue on our GitHub repository.

As our contracts are now fully audited, STK is preparing for our main-net launch. We will keep you all updated on our progress!