[renBTC] ERC20 Token Smart Contract Technical Assessment

[renBTC] ERC20 Token Smart Contract Technical Assessment

General Information

Risk Summary

  • Does the contract implement the ERC20 token standards? Yes.
  • Risk analysis : MEDIUM.

Technical Information

  • Compiler version : v0.5.16+commit.9c3226ce
  • Decimals : 8
  • Overflow checks : Yes, the contract uses the SafeMath library for uint operations.
  • Mitigation against allowance race-condition : Yes, the contract implements increaseAllowance and decreaseAllowance to get around this issue.
  • Upgradeable contract patterns : Yes.
  • Access control or restriction lists : No.
  • Non-standard features or behaviors : Yes.
    • Two EOAs Mint Authority and Owner can mint renBTC.

Formal Verification Considerations:

  • Does transfer have simple semantics? Yes.
  • Does transferFrom have simple semantics? Yes.
  • Can balances be arbitrarily modified by some actor? No.
  • Are there any external calls? No.

Testnet Information

  • renBTC is deployed on Kovan. List of relevant addresses here.

Contract Logic Summary

Administrative Addresses

The minting is controlled by an EOA - the Mint Authority.

The proxy implementation can be changed arbitrarily by the timelocked multsig after a 7 day delay.

Inheritance Structure

renBTC uses a bunch of standard OpenZeppelin contracts for the Proxy and ERC20 implementation.

Contract Risk Summary

This is a MEDIUM risk contract. The ERC20 functions are implemented to industry standard and there are checks to prevent over/underflows. The risk comes from minting being controlled by a single EOA, and the upgradable contract pattern. However, implementation changes are behind a 7 day timelock controlled by a multisig, so there is time to react via governance executive.

Supporting Materials

Architecture Diagram

Inheritance Diagram

Sūrya’s Description Report

Files Description Table

File Name SHA-1 Hash
renBTC.sol fc10e6ea816a5b894602dddea5f6c886f199b8f4

Contracts Description Table

Contract Type Bases
└ Function Name Visibility Mutability Modifiers
Initializable Implementation
└ isConstructor Private :closed_lock_with_key:
Context Implementation Initializable
└ Internal :lock: :stop_sign:
└ _msgSender Internal :lock:
└ _msgData Internal :lock:
Ownable Implementation Initializable, Context
└ initialize Public :exclamation: :stop_sign: initializer
└ owner Public :exclamation: NO❗️
└ isOwner Public :exclamation: NO❗️
└ renounceOwnership Public :exclamation: :stop_sign: onlyOwner
└ transferOwnership Public :exclamation: :stop_sign: onlyOwner
└ _transferOwnership Internal :lock: :stop_sign:
Proxy Implementation
└ External :exclamation: :dollar: NO❗️
└ _implementation Internal :lock:
└ _delegate Internal :lock: :stop_sign:
└ _willFallback Internal :lock: :stop_sign:
└ _fallback Internal :lock: :stop_sign:
OpenZeppelinUpgradesAddress Library
└ isContract Internal :lock:
BaseUpgradeabilityProxy Implementation Proxy
└ _implementation Internal :lock:
└ _upgradeTo Internal :lock: :stop_sign:
└ _setImplementation Internal :lock: :stop_sign:
UpgradeabilityProxy Implementation BaseUpgradeabilityProxy
└ Public :exclamation: :dollar: NO❗️
BaseAdminUpgradeabilityProxy Implementation BaseUpgradeabilityProxy
└ admin External :exclamation: :stop_sign: ifAdmin
└ implementation External :exclamation: :stop_sign: ifAdmin
└ changeAdmin External :exclamation: :stop_sign: ifAdmin
└ upgradeTo External :exclamation: :stop_sign: ifAdmin
└ upgradeToAndCall External :exclamation: :dollar: ifAdmin
└ _admin Internal :lock:
└ _setAdmin Internal :lock: :stop_sign:
└ _willFallback Internal :lock: :stop_sign:
InitializableUpgradeabilityProxy Implementation BaseUpgradeabilityProxy
└ initialize Public :exclamation: :dollar: NO❗️
InitializableAdminUpgradeabilityProxy Implementation BaseAdminUpgradeabilityProxy, InitializableUpgradeabilityProxy
└ initialize Public :exclamation: :dollar: NO❗️
IERC20 Interface
└ totalSupply External :exclamation: NO❗️
└ balanceOf External :exclamation: NO❗️
└ transfer External :exclamation: :stop_sign: NO❗️
└ allowance External :exclamation: NO❗️
└ approve External :exclamation: :stop_sign: NO❗️
└ transferFrom External :exclamation: :stop_sign: NO❗️
SafeMath Library
└ add Internal :lock:
└ sub Internal :lock:
└ sub Internal :lock:
└ mul Internal :lock:
└ div Internal :lock:
└ div Internal :lock:
└ mod Internal :lock:
└ mod Internal :lock:
ERC20 Implementation Initializable, Context, IERC20
└ totalSupply Public :exclamation: NO❗️
└ balanceOf Public :exclamation: NO❗️
└ transfer Public :exclamation: :stop_sign: NO❗️
└ allowance Public :exclamation: NO❗️
└ approve Public :exclamation: :stop_sign: NO❗️
└ transferFrom Public :exclamation: :stop_sign: NO❗️
└ increaseAllowance Public :exclamation: :stop_sign: NO❗️
└ decreaseAllowance Public :exclamation: :stop_sign: NO❗️
└ _transfer Internal :lock: :stop_sign:
└ _mint Internal :lock: :stop_sign:
└ _burn Internal :lock: :stop_sign:
└ _approve Internal :lock: :stop_sign:
└ _burnFrom Internal :lock: :stop_sign:
ERC20Detailed Implementation Initializable, IERC20
└ initialize Public :exclamation: :stop_sign: initializer
└ name Public :exclamation: NO❗️
└ symbol Public :exclamation: NO❗️
└ decimals Public :exclamation: NO❗️
Claimable Implementation Initializable, Ownable
└ initialize Public :exclamation: :stop_sign: initializer
└ transferOwnership Public :exclamation: :stop_sign: onlyOwner
└ claimOwnership Public :exclamation: :stop_sign: onlyPendingOwner
Address Library
└ isContract Internal :lock:
└ toPayable Internal :lock:
└ sendValue Internal :lock: :stop_sign:
SafeERC20 Library
└ safeTransfer Internal :lock: :stop_sign:
└ safeTransferFrom Internal :lock: :stop_sign:
└ safeApprove Internal :lock: :stop_sign:
└ safeIncreaseAllowance Internal :lock: :stop_sign:
└ safeDecreaseAllowance Internal :lock: :stop_sign:
└ callOptionalReturn Private :closed_lock_with_key: :stop_sign:
CanReclaimTokens Implementation Claimable
└ initialize Public :exclamation: :stop_sign: initializer
└ blacklistRecoverableToken Public :exclamation: :stop_sign: onlyOwner
└ recoverTokens External :exclamation: :stop_sign: onlyOwner
ERC20WithRate Implementation Initializable, Ownable, ERC20
└ initialize Public :exclamation: :stop_sign: initializer
└ setExchangeRate Public :exclamation: :stop_sign: onlyOwner
└ exchangeRateCurrent Public :exclamation: NO❗️
└ _setRate Internal :lock: :stop_sign:
└ balanceOfUnderlying Public :exclamation: NO❗️
└ toUnderlying Public :exclamation: NO❗️
└ fromUnderlying Public :exclamation: NO❗️
ERC20WithPermit Implementation Initializable, ERC20, ERC20Detailed
└ initialize Public :exclamation: :stop_sign: initializer
└ permit External :exclamation: :stop_sign: NO❗️
RenERC20LogicV1 Implementation Initializable, ERC20, ERC20Detailed, ERC20WithRate, ERC20WithPermit, Claimable, CanReclaimTokens
└ initialize Public :exclamation: :stop_sign: initializer
└ mint Public :exclamation: :stop_sign: onlyOwner
└ burn Public :exclamation: :stop_sign: onlyOwner
└ transfer Public :exclamation: :stop_sign: NO❗️
└ transferFrom Public :exclamation: :stop_sign: NO❗️
RenBTC Implementation InitializableAdminUpgradeabilityProxy
RenZEC Implementation InitializableAdminUpgradeabilityProxy
RenBCH Implementation InitializableAdminUpgradeabilityProxy

Legend

Symbol Meaning
:stop_sign: Function can modify state
:dollar: Function is payable
15 Likes

Thanks for bringing this up, Sam. I’ll add that after looking at the Token Proxy, the _implementation() call has been made internal on the contract, so there’s no way to mitigate this risk with an implementation check in the adapter, either.

5 Likes

We will in the next 24h start the process of moving over the control to a timelock contract. We’ll be using this audited timelock by Compound, and have the minimum delay set at 7 days.

The reason for not implementing a timelock before is out of an abundance of caution, in case we needed to quickly implement a fix or a change. On the other hand our confidence in the security of the system is growing every day and we think that should be reflected in the token contracts as well, so we have no blocker to changing control over to a timelock.

6 Likes

Appreciate the timely response @MaxRoszko. A 7 day timelock is definitely a lot better than what exists now.

What are your thoughts on switching to a multisig to control the implementation as well? It’s a lot of power for a single EOA, and we cannot automatically deactivate on implementation change due to the transparent proxy.

Yes we’ll control the timelock through a standard Gnosis Multisig for now, with plans of decentralizing the multisig setup next year (perhaps through a different system), as we hand more governance over to the REN holders.

6 Likes

Great, thanks for addressing these concerns. I’ll update the assessment after these changes are made.

6 Likes

I’ll keep you guys posted in this thread! :+1:

6 Likes

This has been resolved now, all Ren gateways (so not just the renBTC contract but renZEC, renFIL etc.) are now owned by a timelock contract, and the timelock contract is owned by a Gnosis Multisig! :+1:

7 Likes

Just checked this again, and it looks like the EOA 0x33024cfb7af11a7cb12ab0dedefc5dd5f430381f is still in control of the ProxyAdmin.

The ADMIN_SLOT position on RenBTC points to RenProxyAdmin which has its owner() set to 0x33024cfb7af11a7cb12ab0dedefc5dd5f430381f. Same with BTCGateway.

1 Like

Sorry about this, there was a failed transaction last time that was unnoticed, here is the new successful one: https://etherscan.io/tx/0x8f2b1be5705ea00841947971ea77d9d1e985d88eaac0bc687ce95e5d9b7dea86

4 Likes

Appreciate you making those changes @MaxRoszko. I’ve downgraded the risk to MEDIUM and updated the post to match the new setup.

2 Likes