TrueUSD Proof of Reserve

Functions decreaseAllowance() and transferFrom() of the ERC20 contract do not consider the case of an “infinite” allowance, which is represented by type(uint256).max. This means that even an “infinite” allowance will be decreased to something less than “infinite”. Even though this might not affect the correct function of the protocol, there might be users of the protocol that do not expect this behavior, as new versions of ERC20 have addressed this issue.

[Response of the protocol team: We treat OpenZeppelin’s ERC20 implementation as a reference for our contract. In the latest ERC20.sol transferFrom does not decrease infinite allowance and decreaseAllowance does. We'll consider including this change in the next deployment.]

Two multisig wallets owned by the protocol team, one of which is the owner of the TokenControllerV3 contract who has total control over the protocol, can disable the Proof of Reserve feature at will. Even though improbable, compromise of especially the owner key could result in minting an unlimited amount of TUSD even with the Proof of Reserve feature enabled, as the owner key can be used to disable it. It might be wise to introduce a reasonable time buffer for operations like disableProofOfReserve and setChainReserveFeed(address(0)) to finalize in order to prevent the catastrophic consequences of an owner’s key compromise. Such a buffer would of course add unnecessary delay when there is a legitimate reason to disable the PoR feature (PoR is not functioning or banking partner’s API is down) but would serve as an extra line of defense by giving enough time to the protocol team to react to an attack.

In case the mint key has also the ratifier attribute/role, compromise of the mint key would result in excessive minting, up to the whole ratifiedMintPool, as only one ratifier is required to refill the instantMintPool by withdrawing from the ratifiedMintPool. The protocol team is aware of this danger and is keeping the keys separate intentionally, which we confirmed by querying the attribute/role registry contract. Nevertheless, it would be worth introducing require()s (in the Registry and TokenControllerV3 contracts) to ensure the mint key and ratifiers remain disjoint sets.

[Response of the protocol team: The protocol team is keeping the keys separate.

Registry is a generalized contract used by multiple TokenControllers. To enforce the separation of ratifier and mint key roles in the proposed way, we would need to add checks to Registry contract methods for setting attribute values. This would introduce a cyclic dependency between Registry and those TokenController contracts which we want to avoid. We could probably also move the mintKey role management to the Registry contract and enforce the separation of roles there, avoiding the cyclic dependencies. This is not relevant to the current deployment though, so we decided to not pursue this.]

Function TokenControllerV3::pauseToken uses the onlyFastPauseOrOwner modifier even though fastPause has been deprecated. The modifier can thus be reduced to onlyOwner for the pauseToken function and the onlyFastPauseOrOwner modifier can be completely removed.Another unused modifier, which can be removed, is the onlyOwnerOrRedemptionAdmin modifier.

The function TokenControllerV3::requestMint does not perform any sanity checks on the _to and _value parameters provided to it. If the caller of the function has not performed such checks and one of the parameters has been assigned an invalid value, e.g., _to has been assigned the 0 address, the transaction will revert at a much later point, i.e., in the ERC20::_mint function. This will result in a lot of gas being wasted, as the mint requested will have to be ratified (possibly by multiple ratifiers) before it is finalized.

The function TokenControllerV3::ratifyMint calls function hasEnoughApproval and in case it returns True, finalizeMint is called.

function ratifyMint(
  uint256 _index,
  address _to,
  uint256 _value
) external mintNotPaused onlyMintRatifierOrOwner {
  MintOperation memory op = mintOperations[_index];
  require(op.to == _to, "to address does not match");
  require(op.value == _value, "amount does not match");
  require(!mintOperations[_index].approved[msg.sender],
    "already approved");
  mintOperations[_index].approved[msg.sender] = true;
  mintOperations[_index].numberOfApproval =
    mintOperations[_index].numberOfApproval.add(1);
  emit MintRatified(_index, msg.sender);
  if (hasEnoughApproval(mintOperations[_index].numberOfApproval,_value)) {
    finalizeMint(_index);
    // Dedaub: finalizeMint -> canFinalize -> hasEnoughApproval
  }
}

In turn, finalizeMint calls the canFinalize function that requires hasEnoughApproval to return True for that same mint operation whose approvals must have been already verified by the first call to hasEnoughApproval. Nevertheless, this is not the only operation wasting gas, as in every one of these subcalls the MintOperation data are copied again from storage to memory, something that could be avoided, e.g., by creating internal and more optimized counterparts of the finalizeMint and canFinalize functions.

In several functions (e.g., transferTrueCurrencyProxyOwnership) of TokenControllerV3 the pattern address(uint160(address(token))) is used to cast the address of token to a payable one. In versions 0.6.x of Solidity and forward this can be done by using the more simplistic payable(address(token)).

TrueCurrencyWithProofOfReserve::enableProofOfReserve could implement another require() that ensures the chainReserveHeartbeat has been set.

function enableProofOfReserve() external override onlyOwner {
  require(chainReserveFeed != address(0),
    "TrueCurrency: chainReserveFeed not set");
  // Dedaub: extra require()
  require(chainReserveHeartbeat != 0,
    "TrueCurrency: chainReserveHeartbeat not set");
  proofOfReserveEnabled = true;
  emit ProofOfReserveEnabled();
}

This would ensure that TrueCurrencyWithProofOfReserve::_mint does not fail when using the new reserves feature due to chainReserveHeartbeat not having been set.

The ITrueCurrency interface does not declare the function burn, which is nevertheless implemented by the abstract contract BurnableTokenWithBounds.

The functions transferOwnership and claimOwnership of the ClaimableOwnable contract could be made external instead of public, as they are not called by any of the contract’s (or the contract’s subclasses) functions.

In TrueCurrency.sol it is mentioned that “The first 0x100000 addresses are redemption addresses”. However, a more accurate description would be “The first 0x100000 addresses except from address(0) are redemption addresses”, as the function isRedemptionAddress enforces the exemption of address(0) by requiring uint256(account) != 0. This could be made even more clear by changing the type casts in the aforementioned requirement to account != address(0).

The interface IHook defined in TokenControllerV3.sol remains unused after the deprecation of gas refunder and can thus be removed.

The onlyRegistryAdmin modifier has to be renamed to onlyRegistryAdminOrOwner as the owner of the contract is also allowed by it.

modifier onlyRegistryAdmin() {
  require(registry.hasAttribute(msg.sender, IS_REGISTRY_ADMIN) ||
    msg.sender == owner, "must be registry admin or owner");
  _;
}

The code is compiled with Solidity 0.6.10. Version 0.6.10, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.