Skip to main content
Dedaub

TrueUSD Proof of Reserve

Smart Contract Security Assessment

October 27, 2022

Archblock

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of the new Proof of Reserve feature of TrueUSD.

Proof of Reserve release extends the TrueUSD mint function with a requirement that ensures TUSD token supply never exceeds the USD reserves held in escrow accounts backing TUSD on the particular blockchain. Information about chain reserves is read from a Chainlink Proof of Reserve data feed, which is populated with data provided by Armanino’s real-time attestation.

This audit covers the upgrades of TrueUSD and TokenControllerV2 contracts that are available under the public repository contracts-pre22/packages/contracts-por, at commit hash 39743f81bbe789607856611a5612b2151a08f6b1.

Two auditors worked on the codebase for 3 days on the following contracts:

packages/contracts-por/contracts/
  • TokenControllerV3.sol
  • TrueCurrency.sol
  • TrueCurrencyWithProofOfReserve.sol
  • common/
    • BurnableTokenWithBounds.sol
    • ClaimableOwnable.sol
    • ERC20.sol
    • ProxyStorage.sol
    • ReclaimerToken.sol
  • interface/
    • IOwnedUpgradeabilityProxy.sol
    • IProofOfReserveToken.sol
    • IRegistry.sol
    • ITrueCurrency.sol
  • tokens/
    • TrueUSD.sol

SETTING & CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

[No high severity issues]


MEDIUM SEVERITY

[No medium severity issues]


LOW SEVERITY

L1

ERC20::decreaseAllowance will decrease “infinite” amount

L1LOW

ERC20::decreaseAllowance will decrease “infinite” amount
acknowledged

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.]



CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

N1

The Proof of Reserve feature can be disabled by selected multisig wallets

N1CENTRALIZATION

The Proof of Reserve feature can be disabled by selected multisig wallets
acknowledged

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.

N2

The protocol does not strictly enforce the separation of ratifier and mint keys/roles

N2CENTRALIZATION

The protocol does not strictly enforce the separation of ratifier and mint keys/roles
dismissed

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.]



OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Function modifiers can be removed/simplified

A1ADVISORY

Function modifiers can be removed/simplified
resolved

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.

A2

TokenControllerV3::requestMint does not check the provided parameters

A2ADVISORY

TokenControllerV3::requestMint does not check the provided parameters
info

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.

A3

TokenControllerV3::ratifyMint might call hasEnoughApproval twice with no added benefit

A3ADVISORY

TokenControllerV3::ratifyMint might call hasEnoughApproval twice with no added benefit
info

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.

A4

Code can be simplified

A4ADVISORY

Code can be simplified
resolved

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)).

A5

TrueCurrencyWithProofOfReserve::enableProofOfReserve require()s could be made stricter

A5ADVISORY

TrueCurrencyWithProofOfReserve::enableProofOfReserve require()s could be made stricter
resolved

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.

A6

ITrueCurrency does not declare function burn

A6ADVISORY

ITrueCurrency does not declare function burn
resolved

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

A7

Functions could be made external

A7ADVISORY

Functions could be made external
resolved

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.

A8

Code comment could be more accurate

A8ADVISORY

Code comment could be more accurate
resolved

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).

A9

Unused interface in TokenControllerV3

A9ADVISORY

Unused interface in TokenControllerV3
resolved

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

A10

The onlyRegistryAdmin modifier should be renamed to onlyRegistryAdminOrOwner

A10ADVISORY

The onlyRegistryAdmin modifier should be renamed to onlyRegistryAdminOrOwner
resolved

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");
_;
}

A11

Compiler bugs

A11ADVISORY

Compiler bugs
info

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.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub