# Alethea ERC20 (ALI) - Smart Contract Audit

###### tags: `alethea`, `audit`

Latest revision: July 2nd, 2021
Miguel Palhas <[e-mail]>

## Table of Contents

- [Overview](#Overview)
- [Background](#Background)
- [Dates](#Dates)
- [Coverage](#Coverage)
- [Target Code and Revision](#Target-Code-and-Revision)
- [Documentation](#Documentation)
- [Areas of Concern](#Areas-of-Concern)
- [Findings](#Findings)
- [General Comments](#General-Comments)
- [Specific Issues & Suggestions](#Specific-Issues-amp-Suggestions)

## Overview

### Background

Alethea is a protocol that aims to create a ecosystem of intelligent NFTs (iNFTs). This audit refers to ALI ERC20 token that is part of the system.

### Dates

- **June 29th**: Start date
- **July 2nd**: Delivery of initial repor

### Process

This document, and all suggestions detailed here, is meant to be scrutinized and discussed between both parties, in an ongoing collaborativev process after the first report delivery. Each suggestion may not be needed due to contextual reasons, or limited understanding of external scope by the auditor.

## Coverage

The following code was considered in-scope for the review:
https://github.com/AletheaAI/Gitcoin-Bounties/tree/main/bounty_1
It consists of a Hardhat project containing Solidity code for the ALI token.

Specifically, the latest revision at the time of the review was used: `6858edfbc9a0583e3d61905e0e409f45e229631f`

All file references in this document are relative to the bounty_1 directory of the linked repository.

Any dependencies of the code are considered out of scope for this review.
In addition, some code files were copies of existing code in 3rd party reviews that have themselves already been audited. Those specific files were also not in scope, other than ensuring the code matches the original source.

### Documentation

The following documentation was used during the review process:

- [Alethea AI](https://alethea.ai)
- [EIP-20](https://eips.ethereum.org/EIPS/eip-20)
- [EIP-712](https://eips.ethereum.org/EIPS/eip-712)
- [EIP-165](https://eips.ethereum.org/EIPS/eip-165)
- [EIP-1363](https://eips.ethereum.org/EIPS/eip-1363)
- [EIP-2612](https://eips.ethereum.org/EIPS/eip-2612)
- [EIP-3009](https://eips.ethereum.org/EIPS/eip-3009)

### Areas of Concern

The investigation focused on the following:

- Looking for attack vectors that could impact the intended behaviour of the smart contract
- Checking test coverage, particularly for potentially dangerous scenarios and edge-cases
- Interaction with 3rd party contracts
- Use of solidity best practices

## Findings

### General Comments

#### `AliERC20v1` Contract

A simple ERC20 contract with an initial supply of 10 million tokens, a burn mechanism, and support for the ERC165 standard. Not much else to review here.
I also assume from the naming that this was purely an initial setup, and eventually replaced by `AliERC20v2`.

------

#### `AccessExtension`

An implementation on top of Open Zeppelin's AccessControl logic. This seems to be an early draft of the custom `AccessControl` contract, and is not currently used anywhere in the codebase, so review of this contract was skipped.

------

#### `AccessControl` Contract

An optimized version of the AccessControl logic provided by Open Zeppelin, using a bitwise registry for keeping track of permissions.

Given the sensitivity and complexity of the logic, special care was taken when reviewing it. In particular, the following Rust snippet was used to manually check the bitwise functionality against some edge cases:

fn main() {
  let FULL_PRIVILEGES_MASK = 0b1111;
  let operator     = 0b1011;
  let mut original = 0b1111;
  let desired      = 0b1011;
  let mut target = original;
  
  target |= operator & desired;
  target &= FULL_PRIVILEGES_MASK ^ (operator & (FULL_PRIVILEGES_MASK ^ desired));

    
  println!("operator: {:b}\noriginal: {:b}\ndesired:  {:b}\nresult:   {:b}\n",
    operator,
    original,
    desired,
    target
  );
}

No logical problems were found here.

------

#### `ALiERC20v2` Contract

Implements several EIPs: EIP-20, EIP-712, EIP-1363, EIP-2612, EIP-3009.

It also implements a Compound-inspired governance system.

While reviewing this contract, all of the relevant EIPs were re-read, aiming to check the correctness of the implementation.

Overall, no major issues or security problems were found. The following details were checked more extensively, and deemed correct:

* Correctness of the hardoced EIP-712 type hashes (aiming at checking for copy&paste mistakes);
* Checking function and event signatures against each corresponding EIP;
* Checking potential overflow issues in the voting system, due to the discrepancy of using `uint256` for balances, but `uint192` for voting power. This is correctly covered within the minting logic;
* Checking correctness of signature verification mechanism. In particular, the check against malformed signatures silently failing with a zero address was initially hard to check, but it is covered within the included `ECDSA.sol` contract (which is itself copied from Open Zepellin);
* Usage of EIP-2612 sequential nonces together with EIP-3009 non-sequential nonces;
* Custom delegation logic, which is heavily inspired, although not a direct copy of, Compound's own system;
* Usage of each individual feature flag.

------

#### Test coverage

I found the test suite to be extremely comprehensive, and providing confidence in the correctness of the implementation.
A recent version of the solidity compiler is being used, making it unnecessary to use `SafeMath` or check against integer overflow/underflow.

All potential security edge cases, such as malformed signatures and re-entrancy attacks are covered with the test suite

### Specific Issues & Suggestions

#### Incomplete fix for Multiple Withdrawal Attack on ERC20 Tokens

Several points of the contract make a reference to `Multiple Withdrawal Attack on ERC20 Tokens (ISBN:978-1-7281-3027-9)`.

The main public documentation and proposed solution for this attack vector, which is linked within the comments, is this [Google Doc](https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit).

The solution proposed in the spec is two-fold:

1. Adding a new "Compare and Set" approve method:

function approve(
  address _spender,
  uint256 _currentValue,
  uint256 _value)
returns (bool success)

2. Separate Log Message for "TransferFrom" Transfers

The second step is implemented. But by itself, it only serves to make usage of `approve` and `transferFrom` methods more convenient and less error-prone.

The first point, which implements a new atomic approval function, which takes `_currentValue` as an argument to check, is not implemented.

Related to this, line 501 of the `AliERC20v2.sol` contains the following comment:

	 * @dev Fired in approve() and approveAtomic() functions

This hints at an `approveAtomic` function, which I would presume implements this same behaviour. This function is however not present in the code.

**Mitigation**: Implement the 4-arity `approve` function as suggested in the original attack vector [spec document](https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit).

------

#### No ERC-165 support for AccessControl

**Severity**: Notice

There is no interface for `AccessControl` (the original Open Zeppelin implementation provides an `IAccessControl`), which means `supportsInterface()`
does not know about it. Not necessarily a problem, as it may not be
necessary to publicize support for the `AccessControl` interface.

**Mitigation**: create `IAccessControl`, and add that to the implementation of `supportsInterface` in `AliERC20v2`.

------

#### Incorrect comment for `Approved` event

**Severity**: Notice

The comments for `event Approved` state `Fired in approve() and approveAtomic() functions`.
However, this event is also potentially fired within `__transferFrom` `burn`.
From context, it's easy to understand that those firings seem correct, and the comment is probably outdated, but still worth pointing out the discrepancy.
Outdated comments may cause invalid expectations from anyone reading them and consuming events.

**Mitigation**: Check whether the current functionality is actually intended, and update the comment accordingly.

------

#### Inconsistent naming for extra ERC20 events

**Severity**: Notice

The contract implements several small updates to the ERC20 standard, intended to mitigate the Multiple Withdrawal Attack on ERC20 Tokens. In particular, the comments link to a Github comment, which then links to the original Google Docs spec of the attack vector:

- Github Comment: https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
- Attack Vector spec doc: https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

The proposed solution in the spec includes adding two new events:

event Transfer(
  address indexed _spender,
  address indexed _from,
  address indexed _to,
  uint256 _value)
  
event Approval(
  address indexed _owner,
  address indexed _spender,
  uint256 _oldValue,
  uint256 _value)

These events have the same name as the 3-arity counterpart in the original ERC-20 spec (solidity [does allow](https://github.com/ethereum/solidity/issues/5970) for event overloading).

However, in the code they were renamed to `Transferred` and `Approved`, which creates inconsistencies with the fix that was originally being followed, and breaks expectations for consumers of the events.

**Mitigation**: Rename events to match the spec provided in the original proposal.

------

#### Consider increasing optimizer runs

**Severity**: Low

The `solc` optimizer is set for the default `runs: 200`. This value is meant as a measure of how many transactions the contract is expected to receive.
The rule of thumb for high-activity contracts such as ERC20 tokens is to increase this value as much as possible. This tends to increases the bytecode and initial cost of deploy, but decrease overall gas costs for users interacting with it.

While testing, increasing the value to `runs: 1000` seems to have yielded savings of about 100 gas per transaction call, and `runs: 2000` further decreased it, although with diminishing returns.

Since this has implications on deploy costs, even though it's a one-time expense, it should be considered carefully, and it's not necessarily a must-change.

PS: [hardhat-gas-reporter](https://hardhat.org/plugins/hardhat-gas-reporter.html) was used to get statistics on overall gas usage for each setting