Security review of new tokens

[pyggie]

Over in another thread (Five new tokens to add (October 2020)) we are discussing which tokens to add to the money market protocol.

Each token that we add needs to have a security review of any token functionality that goes beyond the basic ERC20 interface. Just because a token is ERC20 compliant doesn’t mean that it can be used in Percent. There have been multiple exploits already of other protocols due to token behavior that goes beyond ERC20. The most relevant for Percent is the case of imBTC at Lendf.me, where re-entrancy behavior allowed an attacker to completely drain all protocol assets. (The attacker was doxxed by bad opsec and returned the funds.) Another example was the inflationary behavior of STA at Balancer.

Most of the tokens under consideration are already available in one of Compound, Aave, or Cream, which suggests they are probably safe but isn’t a guarantee. New tokens that are unique to Percent would require the most careful inspection.

References:

Uniswap/Lendf.Me Hacks: Root Cause and Loss Analysis

Balancer Pool with STA Deflationary Token Incident

[pyggie]

The current proposal for new tokens:

Snapshot

proposes adding the following:

COMP, UNI, MKR, LRC, sUSD, TUSD, BUSD

We should review each of these for compatibility with Percent’s CToken contracts. We can leverage the work done by Compound and other forks like Cream, plus related projects like Aave. We can’t give a green light just because another project supports a token, but seeing their work can help guide ours.

Where tokens are already supported:
COMP - Compound, Cream
UNI - Compound, Cream
MKR - Aave
LRC - none
sUSD - Aave
TUSD - Aave
BUSD - Aave, Cream

Looking at the token contract sources, I notice the following:

COMP

• no obvious issues
• https://etherscan.io/address/0xc00e94cb662c3520282e6f5717214004a7f26888#code

UNI

• no obvious issues
• https://etherscan.io/address/0x1f9840a85d5af5bf1d1762f925bdaddc4201f984#code

MKR

• no issues: this is a very basic token
• https://etherscan.io/address/0x9f8f72aa9304c8b593d555f12ef6589cc3a579a2#code

LRC

• no issues: this is a very basic token
• https://etherscan.io/address/0xbbbbca6a901c926f240b89eacb641d8aec7aeafd#code

sUSD

• is an upgradable proxy
• proxy: https://etherscan.io/address/0x57ab1ec28d129707052df4df418d58a2d46d5f51#code
• implementation: https://etherscan.io/address/0x705142e6f3970f004721bdf05b696b45fc4ad6d7#code

TUSD

• is an upgradable proxy
• has a _beforeTokenTransfer() hook, but it appears to be empty and not overridden in the concrete subclass
• explanation of hooks: https://docs.openzeppelin.com/contracts/3.x/extending-contracts#using-hooks
• has a blacklist under control of the contract owner
• proxy: https://etherscan.io/address/0x0000000000085d4780B73119b644AE5ecd22b376#code
• implementation: https://etherscan.io/address/0xffc40f39806f1400d8278bfd33823705b5a4c196#code

BUSD

• is an upgradable proxy
• has a blacklist under control of the contract owner
• proxy: https://etherscan.io/address/0x4fabb145d64652a948d72533023f6e7a623c7c53#code
• implementation: https://etherscan.io/address/0x5864c777697bf9881220328bf2f16908c9afcd7e#code

In the case of upgradable proxies, it may be possible for the owner to change the behavior of the transfer() and transferFrom() methods to arbitrary logic. This could potentially create a vulnerability like the one that affected Lendf.me.

A token with a blacklist could block the Percent pToken supply pool from transfers, effectively seizing the tokens it contains.

I don’t see any show-stoppers, but recommend an expert Solidity dev to review each of these.

[Bluecrypt]

Thank you @pyggie for the preliminary analysis, which sets the ground for someone like @splix to give his thoughts on.