Pillar

1 Executive Summary

This report presents the results of our engagement with Pillar Project to review Pillar accounts, wallets, and payment network.

The review was conducted over two weeks, from November 23, 2020 to Dec 4, 2020 by Shayan Eskandari and Sergii Kravchenko.

2 Scope

Our review focused on the commit hash 55ba306582a007b6e0f9c35bdba92a94dc85829c. The list of files in scope can be found in the Appendix.

Even though the files in the scope were clear and the solidity code is well written, the lack of documentation and work flow specification (off-chain with on-chain components) resulted in less in depth review of the overall system.

Update January 2021: Pillar team has worked on the issues reported in this review and their comments has been added to each section. Note that the fixes were not reviewed by ConsenSys Diligence.

3 Recommendations

3.1 Usage of re-entrancy guard  Won't Fix

Resolution

Comment from the client: Additional protection is not needed.

Description

We were not able to find any dangerous exploits or work flows that might be caused by re-entrancy from _executeAccountTransaction or executeTransaction. However, as the accounts will be calling external, user-specified accounts (which might be smart contracts), implementing a re-entrancy guard will add additional protection to important components. This is more crucial for batchSend or delegated sends, as the user sending the transaction is not the owner of the account and might have different permissions in the system than a single user.

3.2 Error messages improve code readability ✓ Fixed

Resolution

Comment from the client: Error messages have been added

Description

Adding error messages to inline conditions such as require() adds readability and makes future debugging easier. It is recommended to add either error codes or explicit error messages to all require conditions.

Examples

    require(
      to != address(0),
      "cannot send to address 0x0"
    );

    require(
      to != address(this),
    "cannot send to the contract address"
    );

4 Trust Model

In any system, it’s important to identify what trust is expected/required between various actors. For this audit, we established the following trust model:

  • The system is semi-trusted. The main point of centralization is the guardians’ mechanism. The guardians are signing messages that are needed to commit a payment channel to the smart contract. When doing so, they are checking that the sender account has enough funds to make the payment. If the guardians go offline, users are still able to withdraw funds using a complex mechanism. The main risk is that if a guardian goes malicious, it can approve double-spending, resulting in stealing some funds.

5 Findings

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

5.1 Delegated transactions can be executed for multiple accounts Major ✓ Fixed

Resolution

Comment from the client: The issue has been solved

Description

The Gateway contract allows users to create meta transactions triggered by the system’s backend. To do so, one of the owners of the account should sign the message in the following format:

code/src/gateway/Gateway.sol:L125-L131

address sender = _hashPrimaryTypedData(
  _hashTypedData(
    nonce,
    to,
    data
  )
).recoverAddress(senderSignature);

The message includes a nonce, destination address, and call data. The problem is that this message does not include the account address. So if the sender is the owner of multiple accounts, this meta transaction can be called for multiple accounts.

Recommendation

Add the account field in the signed message or make sure that any address can be the owner of only one account.

5.2 Removing an owner does not work in PersonalAccountRegistry Major ✓ Fixed

Resolution

Comment from the client: The issue has been solved

Description

An owner of a personal account can be added/removed by other owners. When removing the owner, only removedAtBlockNumber value is updated. accounts[account].owners[owner].added remains true:

code/src/personal/PersonalAccountRegistry.sol:L116-L121

accounts[account].owners[owner].removedAtBlockNumber = block.number;

emit AccountOwnerRemoved(
  account,
  owner
);

But when the account is checked whether this account is the owner, only accounts[account].owners[owner].added is actually checked:

code/src/personal/PersonalAccountRegistry.sol:L255-L286

function _verifySender(
  address account
)
  private
  returns (address)
{
  address sender = _getContextSender();

  if (!accounts[account].owners[sender].added) {
    require(
      accounts[account].salt == 0
    );

    bytes32 salt = keccak256(
      abi.encodePacked(sender)
    );

    require(
      account == _computeAccountAddress(salt)
    );

    accounts[account].salt = salt;
    accounts[account].owners[sender].added = true;

    emit AccountOwnerAdded(
      account,
      sender
    );
  }

  return sender;
}

So the owner will never be removed, because accounts[account].owners[owner].added will always be `true.

Recommendation

Properly check if the account is still the owner in the _verifySender function.

5.3 The withdrawal mechanism is overcomplicated Medium ✓ Fixed

Resolution

Comment from the client:

The withdrawal mechanism has been refactored. In current version user can withdraw funds from the deposit account in two ways: * with guardian signature - withdrawDeposit * using “deposit exit” process

Description

To withdraw the funds, anyone who has the account in PaymentRegistry should call the withdrawDeposit function and go through the withdrawal process. After the lockdown period (30 days), the user will withdraw all the funds from the account.

code/src/payment/PaymentRegistry.sol:L160-L210

function withdrawDeposit(
  address token
)
  external
{
  address owner = _getContextAccount();
  uint256 lockedUntil = deposits[owner].withdrawalLockedUntil[token];

  /* solhint-disable not-rely-on-time */

  if (lockedUntil != 0 && lockedUntil <= now) {
    deposits[owner].withdrawalLockedUntil[token] = 0;

    address depositAccount = deposits[owner].account;
    uint256 depositValue;

    if (token == address(0)) {
      depositValue = depositAccount.balance;
    } else {
      depositValue = ERC20Token(token).balanceOf(depositAccount);
    }

    _transferFromDeposit(
      depositAccount,
      owner,
      token,
      depositValue
    );

    emit DepositWithdrawn(
      depositAccount,
      owner,
      token,
      depositValue
    );
  } else {
    _deployDepositAccount(owner);

    lockedUntil = now.add(depositWithdrawalLockPeriod);

    deposits[owner].withdrawalLockedUntil[token] = lockedUntil;

    emit DepositWithdrawalRequested(
      deposits[owner].account,
      owner,
      token,
      lockedUntil
    );
  }
  /* solhint-enable not-rely-on-time */
}

During that period, everyone who has a channel with the user is forced to commit their channels or lose money from that channel. When doing so, every user will reset the initial lockdown period and the withdrawer should start the process again.

code/src/payment/PaymentRegistry.sol:L479-L480

if (deposits[sender].withdrawalLockedUntil[token] > 0) {
  deposits[sender].withdrawalLockedUntil[token] = 0;

There is no way for the withdrawer to close the channel by himself. If the withdrawer has N channels, it’s theoretically possible to wait for up to N*(30 days) period and make N+2 transactions.

Recommendation

There may be some minor recommendations on how to improve that without major changes:

  • When committing a payment channel, do not reset the lockdown period to zero. Two better option would be either not change it at all or extend to now + depositWithdrawalLockPeriod

5.4 A malicious guardian can steal funds Medium  Won't Fix

Resolution

Comment from the client: The etherspot payment system is semi-trusted by design.

Description

A guardian is signing every message that should be submitted as a payment channel update. A guardian’s two main things to verify are: blockNumber and the fact that the sender has enough funds.

There are two main attack vectors for the malicious guardian:

  • It’s possible to conspire with the previous owner of the account and submit the old blockNumber. This allows them to drain the account.

  • A guardian can also conspire with the sender and send more funds to multiple channels than funds in the account.

Recommendation

Reduce the system’s reliance on single points of failure like the guardians.

5.5 Upgrade solidity version Minor ✓ Fixed

Resolution

Solidity version has been upgraded to 0.6.12

Description

The current minimal solidity version is 0.6.0. But some parts of the code use features from the later versions of solidity, like the high-level version of CREATE2 to create accounts.

Recommendation

Upgrade solidity version to the latest stable (0.6.12).

5.6 The lockdown period shouldn’t be extended when called multiple times Minor ✓ Fixed

Resolution

Comment from the client: The issue has been solved

Description

In order to withdraw a deposit from the PaymentRegistry, the account owner should call the withdrawDeposit function and wait for depositWithdrawalLockPeriod (30 days) before actually transferring all the tokens from the account.

The issue is that if the withdrawer accidentally calls it for the second time before these 30 days pass, the waiting period gets extended for 30 days again.

code/src/payment/PaymentRegistry.sol:L170-L199

if (lockedUntil != 0 && lockedUntil <= now) {
  deposits[owner].withdrawalLockedUntil[token] = 0;

  address depositAccount = deposits[owner].account;
  uint256 depositValue;

  if (token == address(0)) {
    depositValue = depositAccount.balance;
  } else {
    depositValue = ERC20Token(token).balanceOf(depositAccount);
  }

  _transferFromDeposit(
    depositAccount,
    owner,
    token,
    depositValue
  );

  emit DepositWithdrawn(
    depositAccount,
    owner,
    token,
    depositValue
  );
} else {
  _deployDepositAccount(owner);

  lockedUntil = now.add(depositWithdrawalLockPeriod);

Recommendation

Only extend the waiting period when a withdrawal is requested for the first time.

5.7 Missing documentation Minor ✓ Fixed

Resolution

Comment from the client: Code has been documented - We will work on white paper, graphs later

Description

The code base as is, is missing proper documentations to understand the code work flow and logic. The most important pieces are high-level diagrams, user work flows, and updated white paper.

It is important for readability and maintainability of the codebase to add in-line documentations. The Pillar code base under the audit lacks any type of inline documentation and it makes the code reviewer’s job much harder. We highly recommend to provide inline documentation using Solidity’s natspec format, as this will be easier to maintain.

As an example PaymentRegistry.sol without the documentation is really hard to read and understand. There are many assumptions or off-chain dependencies and it’s impossible to understand the flows simply by reading the solidity code.

5.8 Gateway can call any contract Minor  Acknowledged

Resolution

Comment from the client: That’s right Gateway can call any contract, we want to keep it open for any external contract.

Description

The Gateway contract is used as a gateway for meta transactions and batched transactions. It can currently call any contract, while is only intended to call specific contracts in the system that implemented GatewayRecipient interface:

code/src/gateway/Gateway.sol:L280-L292

  for (uint256 i = 0; i < data.length; i++) {
    require(
      to[i] != address(0)
    );

    // solhint-disable-next-line avoid-low-level-calls
    (succeeded,) = to[i].call(abi.encodePacked(data[i], account, sender));

    require(
      succeeded
    );
  }
}

There are currently no restrictions for to value.

Recommendation

Make sure, only intended contracts can be called by the Gateway : PersonalAccountRegistry, PaymentRegistry, ENSController.

5.9 Remove unused code Minor ✓ Fixed

Resolution

Comment from the client: Unused code has been removed

Description

In account/AccountController.sol when deploying an account, the function _deployAccount() gets an extra input value which is always 0 and not set in any other method.

Examples

code/src/common/account/AccountController.sol:L24-L38

  return _deployAccount(
    salt,
    0
  );
}

function _deployAccount(
  bytes32 salt,
  uint256 value
)
  internal
  returns (address)
{
  return address(new Account{salt: salt, value: value}());
}

Recommendation

It is recommended to remove this value as there are no use cases for it at the moment, however if it is planned to be used in the future, it should be well documented in the code to prevent confusion.

5.10 Using ENS subdomains introduces possible privacy issues Minor  Acknowledged

Resolution

Comment from the client: This is a known issue - we also added releaseNode in a case we want to move ownership of ens root node

Description

Using ENS names by default introduces a privacy issue for users. The current implementation leaks all user addresses and their associated username. This is possibly known issue, however it is worth to mention as part of this audit.

Examples

Here’s a sample of already registered addresses on mainnet fetched using Legions:

sbeta> ens listSubdomains name="pillar.eth"
> Subdomains for 'pillar.eth'
> NameHash: '0x5bb02333b1f96385ba28fd63408843cfeee095b32196b718786a56e491e33387'
Subdomain Owner
mrsirio 0x6d2ce500f82e20cdeb733ec0530360d2e761f44d
coinstacker 0x60cc065f860682fb899a385b9af66fe82b412b29
dadang 0x904e88eb2602d947ded5c0c5b84c32109255a5f2
ramaido 0x1ee590464e00780ab1c620de41545e74c0731521
tongkol 0x3cbbf43f7a449d54a71bf97c779186f183d1e9eb
kell 0x3d48c65ddfb5bed5980b40974416b55eceed6fab
sipa 0x944972562ea6a07ee0f77bf6ce89559214347774
joyboy 0x4660b09e45930d5ffaedf36bad4a37705303970b
ryanc 0x0c58b9d8b6bdfcd7fb33ab1ecc6b0db4fa94a7b8
hammad 0xe94bb8ea91bfa791cf632e2353cabb87a93713d6
nicolas 0x12ce0a744ccf8958b6859aff1e85bca797e4f742
timmy2shoes 0xafad99c454d97b0130da64179e1a5a7b516ae225
sergvind 0xd5164fe7b9b1d44dd4eb35ef312ada6bce2878ff
0x7384e49fdf540de561f0dc810cc9ad87e909afbe
0x2e496c59c5a0f525d82cf0402851f361ac879c63

Appendix 1 - Files in Scope

This audit covered the following files:

  • ./src/account/AccountOwnerRegistry.sol
  • ./src/account/AccountProofRegistry.sol
  • ./src/common/access/Controlled.sol
  • ./src/common/access/Guarded.sol
  • ./src/common/account/Account.sol
  • ./src/common/account/AccountController.sol
  • ./src/common/libs/BlockLib.sol
  • ./src/common/libs/BytesLib.sol
  • ./src/common/libs/SafeMathLib.sol
  • ./src/common/libs/SignatureLib.sol
  • ./src/common/lifecycle/Initializable.sol
  • ./src/common/token/ERC20Token.sol
  • ./src/common/typedData/TypedDataContainer.sol
  • ./src/ens/ENSController.sol
  • ./src/ens/ENSRegistry.sol
  • ./src/gateway/Gateway.sol
  • ./src/gateway/GatewayRecipient.sol
  • ./src/gateway/GatewayRecipientMock.sol
  • ./src/payment/PaymentRegistry.sol
  • ./src/personal/PersonalAccountRegistry.sol
  • ./src/utils/BalanceChecker.sol

Appendix 2 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.