[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