Carbon Money Smart Contract Audit

Carbon-12 Labs engaged New Alchemy to audit Carbon’s Crypto-Fiat gateway set of smart contracts. These contracts consist of twenty-six custom source files, reference multiple standardized external repositories and are supported by a testing environment containing a significant amount of test cases.

The engagement was technical in nature and focused on identifying security flaws in the design and implementation of the contracts, finding differences between the contract’s implementation and their behavior as described in public documentation, and finding any other issues with the contracts that may impact their trustworthiness. Carbon provided New Alchemy with access to a detailed whitepaper and a GitHub repository containing the source code with an associated README.md file.

The audit was performed over nine days. This document describes the issues discovered in the audit.

Note: The initial version of this document was provided to Carbon-12 Labs who then made various changes to their smart contract source code based upon New Alchemy’s findings. This document now consists of the original and unchanged audit report (v1.0) overlaid with re-test results (v2.0). Re-test results are reflected in each issue title as ‘Fixed’, ‘Partially Fixed’, ‘Not Fixed’ or ‘Informational’. Supporting re-test commentary is attached to a bolded ‘Re-test v2.0:’ prefix and placed at the end of the relevant section. The bulk of the re-test content relates to an examination of individual issues, along with brief comments in the executive summary and files audited sections. All figures are from the initial version of the document.

Carbon’s Crypto-Fiat gateway set of smart contracts consist of twenty-six custom source files, reference multiple standardized external repositories and are supported by a testing environment containing a significant amount of test cases. The contract code demonstrated excellent development techniques in that modern principles such as inheritance and composition were used, standardized and well tested libraries were leveraged, and the testcases considered both positive and negative scenarios.

The audit uncovered no critical issues. However, the audit did uncover five moderate issues and five minor issues, primarily involving:

• A missing SafeMath usage instance that can result in a silent integer overflow.
• Missing Pausable usage instances that can result in continued (partial) contract operation.
• Hardcoded status return values suggesting inconsistent error checking/handling.
• Common ERC-20 weaknesses that can result in double spending allowances.
• Concerns regarding complexity of contract ownership structure and redundant inheritance.

It should be noted that the extensive use of proxy functionality allows these contracts to be updated should changes be desired. However, this functionality also removes the guarantee of contract immutability, so users must realize, accept and trust that contract owners have the ability change anything at any time.

While the test cases were not in scope, they were helpful to understand and explore issues. The whitepaper and README.md were considered the primary documentation for critical constants and intended functionality. Since large sections of the base functionality is standard and leverages widely-used and reviewed contracts, most of the findings pertained to the code found in the custom contract files.

New Alchemy believes the way forward should include contract source corrections corresponding to the issues identified in this review, development of additional documentation targeting the contract design, implementation and operation, and user documentation for mitigating the known ERC-20 weaknesses. The absence of critical findings is a very positive leading indicator.

Re-test v2.0: New Alchemy has discussed the prior audit results with Carbon-12 Labs, advised on efficient and effective mitigation approaches, and inspected the resulting smart contract code provided for re-testing. New Alchemy has concluded:

• All issues have been addressed and the associated risks effectively mitigated.
• The single minor issue labeled ‘Informational’ below is related to short-address attacks which are contentious among developers, the community is trending towards accepting rather than mitigating this risk, and mitigation can only occur at the source outside of the audited contract code. New Alchemy considers Carbon-12 Lab’s approach appropriate and sufficient.

Files Audited

The code reviewed by New Alchemy is in the private GitHub repository https://github.com/stablecarbon/fiat_gatewayat commit hash 4502e04625171f28b5dbdeef65e1f734831487ab.

https://github.com/stablecarbon/fiat_gateway/tree/4502e04625171f28b5dbdeef65e1f734831487ab/contracts
  Migrations.sol                                              4504ea8a24ad6ccde61e98954ef7adf0a714369a
  regulator/carbonDollarRegulator/CarbonDollarRegulator.sol   b864526bfa3b1e1fc14958d972abc6bb1f255784
  regulator/dataStorage/MutableRegulatorStorage.sol           697a19b0158cf8298db8379eb31462df0416cef6
  regulator/dataStorage/PermissionSheet.sol                   ad798e2317c7e7f3a07d1842966e10aa954fc3e0
  regulator/dataStorage/RegulatorStorage.sol                  d2f9a81057b04fad9d0fb823d772a1cbf0de7ff9
  regulator/dataStorage/ValidatorSheet.sol                    c70420abcf4bff542797c414bae9219a0bc7d170
  regulator/mocks/PermissionSheetMock.sol                     580fa867015dcad949a587ab574870131d5968b7
  regulator/mocks/ValidatorSheetMock.sol                      05cb25477d08ceb813b574b50b693af7b948f2bf
  regulator/RegulatorProxy.sol                                8311a70d7d191f789e6a9c119e2bc9a264610d85
  regulator/Regulator.sol                                     1d098f43d3a67f36e08eae39f36d342362c1aefd
  regulator/whitelistedRegulator/WhitelistedTokenRegula...sol ec4afe50d43b3d7759c5bf1a204cee8e017c7464
  tokens/carbonToken/CarbonDollarProxy.sol                    bcacd7aabea4a8947cd83952dbbef865919ece44
  tokens/carbonToken/CarbonDollar.sol                         83c5a05f97568b6d2a7617356d8ec73af76fe7a5
  tokens/carbonToken/dataStorage/CarbonDollarStorage.sol      b50ffc94709b0e516644147d99e09c1a425f9c97
  tokens/carbonToken/dataStorage/FeeSheet.sol                 6c6f978fee8ec3700136ff400cfd8693cb860900
  tokens/carbonToken/dataStorage/MutableCarbonDollarSto...sol ec99e50b13d48f3c754638fe634b4c6bf0e4930f
  tokens/carbonToken/dataStorage/StablecoinWhitelist.sol      a26657e359535c714bfe91e9c4a70fbe90e4cf94
  tokens/permissionedToken/dataStorage/AllowanceSheet.sol     5e3b21392ef10a906bcdfaf1a3606430c72ea0ad
  tokens/permissionedToken/dataStorage/BalanceSheet.sol       ada48381955180cc7afedd48000558120decbf7d
  tokens/permissionedToken/dataStorage/MutablePermissio...sol a7bb7dcde16761acac45bf159ac002591c756e4f
  tokens/permissionedToken/dataStorage/PermissionedToke...sol d610f5b91304cdeda6327a4f422cdd4069bcf696
  tokens/permissionedToken/PermissionedTokenProxy.sol         7843330241df785d4218288653ae72376a9dc303
  tokens/permissionedToken/PermissionedToken.sol              f1f91a3531784a807a6082d2971870703b104ec7
  tokens/whitelistedToken/WhitelistedToken.sol                2ec1237dadca7432fff760e98fa805351054063a
  upgradeability/DelayedUpgradeabilityProxy.sol               cbf189dc3841a8becac2bdb1b79af1300fa0e56e
  upgradeability/mocks/DelayedUpgradeabilityProxyMock.sol     08f2f63d577a03b052c033fa0af01a5af66f09a8
  upgradeability/mocks/DummyContract.sol                      e143161f4c70ce69b38a544ad20adce1ff87a6e6
$ npm list openzeppelin-solidity
  carbonusd@1.0.0 /home/--/work/ts/carbon/fiat_gateway
  |-- openzeppelin-solidity@1.11.0 
  |-- zos-lib@1.3.0
    |-- openzeppelin-solidity@1.10.0 
  openzeppelin-solidity/contracts/AddressUtils.sol            129766a57180dcd1912986050421bdc6939f95ea
  openzeppelin-solidity/contracts/lifecycle/Pausable.sol      984f46f5e56b9a4f1c8f457557e8e6eefa294d74
  openzeppelin-solidity/contracts/math/SafeMath.sol           4b468f2d4ff3ff732c5618740319113bc3505828
  openzeppelin-solidity/contracts/ownership/Ownable.sol       2e3d8382f7f1d9872a263a980ecbb577b745123a
  openzeppelin-solidity/contracts/token/ERC20/ERC20.sol       26fd185ce7325b641cc960c5548df5fbf11079e0
  openzeppelin-solidity/contracts/token/ERC20/ERC20Basic.sol  628c5212045740a0ba1f416934768bd6878e3cf6
  zos-lib/contracts/upgradeability/Proxy.sol                  36fbf0b4826c17a8d7b1d64a8181dc92a4689253
  zos-lib/contracts/upgradeability/UpgradeabilityProxy.sol    8e55edbe3158bcc2340382c975919cc9984449d3

New Alchemy’s audit was additionally guided by the whitepaper at https://www.carbon.money/whitepaper.pdf[^wp1]

Re-test v2.0: New Alchemy performed the retest on the smart contracts located in https://github.com/stablecarbon/fiat_gateway/tree/a94b5204e52c9713faa6ddca395e03af1112fe15.

[^wp1]: shasum fdf74b46c7c5cff10de8fbe645fa9897c0ad0c34

General Overview and Discussion

Carbon’s Crypto-Fiat gateway set of smart contracts implement four major areas of functionality — tokens, regulators, data storage and proxy support. Each will be described in detail below. The contracts leverage standard OpenZeppelin functionality around ERC-20, Pausable, Ownable and SafeMath. The emerging standard zos-lib from OpenZeppelin is leveraged for proxy support.

As mentioned in the executive summary, the extensive use of proxy functionality allows these contracts to be updated should changes be desired. However, this functionality also removes the guarantee of contract immutability, so users must realize, accept and trust that contract owners have the ability change anything at any time.

Tokens

The PermissionedToken.sol contract implements custom ERC-20 logic that includes the capability of regulatory checks. It inherits MutablePermissionedTokenStorage.sol for storage (as described below) and defines a significant number of modifiers involving permissions, whitelists, and blacklists. It effectively wraps the ERC-20 functionality with these modifiers while adding specialized functions relating to minting, burning and blacklisted activities.

The WhitelistedToken.sol inherits from PermissionedToken.sol and adds CarbonUSD minting and and WT0 conversion functions. This contract only declares one additional contract variable beyond what it inherits.

Finally, the CarbonDollar.sol contract provides the main functionality for the CarbonUSD meta token. It inherits from both PermissionedToken.sol and MutableCarbonDollarStorage.sol, while providing support for a stablecoin whitelist, managing fees, minting and conversion.

Note that the lowest-level data storage structures are provided to the above contracts as pre-constructed contract instances.

Regulators

The Regulator.sol contract is intended to support securities regulations by ensuring that PermissionedTokens make compliant transfers. It inherits MutableRegulatorStorage.sol for storage (as described below) and provides specialized functions for managing validators along with a wide variety of permissions. These permissions support activities for blacklisted, whitelisted and nonlisted users.

The Regulator.sol contract is then inherited by WhitelistedTokenRegulator.sol which adapts permissions for blacklisted, whitelisted, and nonlisted users. Additional capabilities are provided around stablecoin to CUSD conversion, as well as minting CUSD directly through a specific WT.

The Regulator.sol contract is also inherited by CarbonDollarRegulator.sol which adapts permissions and abilities in a similar fashion.

Note that the primary data storage structures are provided as pre-constructed contract instances.

Storage Structures

The top-level tokens and regulators described above each inherit specialized structures to contain pre-existing instances of relevant base data structures. These base structures (AllowanceSheet.sol, BalanceSheet.sol, FeeSheet.sol, PermissionSheet.sol, StablecoinWhitelist.sol and ValidatorSheet.sol) are deployed in advance and provided to the top-level constructors. Each inherits from Ownable and thus may have different ownership. These base data structures and the specialized structures to contain them are described below.

Regulators

The PermissionSheet.sol contract serves as the lowest-level data structure serving all of the token-level permissions that a user can have. It contains three mappings, four events, eight public constants and five access functions.

The lowest-level ValidatorSheet.sol contract stores validators who are capable of modifying user permissions. It contains one mapping, two events and three access functions.

The RegulatorStorage.sol contract contains one pre-constructed instance each of PermissionShee t.sol and ValidatorSheet.sol provided at construction time with no mechanism for subsequently changing them. This contract's only usage is to be inherited by MutableRegulatorStorage.sol which provides mechanisms for changing each of the two instances noted above.

The MutableRegulatorStorage.sol contract is then inherited by Regulator.sol and RegulatorProxy.sol. The former contract is finally inherited by WhitelistedTokenRegulator.sol and CarbonDollarRegulator.sol.

Permissioned/Whitelisted Tokens

The lowest-level AllowanceSheet.sol contract is a wrapper around an allowance mapping. It contains one mapping, no events and three access functions.

The lowest-level BalanceSheet.sol contract is a wrapper around a balanceOf mapping. It contains one mapping, no events and three access functions.

The PermissionedTokenStorage.sol contract contains one pre-constructed instance each of Regulator.sol, BalanceSheet.sol and AllowanceSheet.sol provided at construction time with no mechanism for subsequently changing them. This contract's only usage is to be inherited by MutablePermissionedTokenStorage.sol which provides mechanisms for changing each of the three instances noted above.

The MutablePermissionedTokenStorage.sol is then inherited by PermissionedToken.sol and PermissionedTokenProxy.sol. The former contract is finally inherited by WhiteListedToken.sol and CarbonDollar.sol.

CarbonDollar

The FeeSheet.sol contract contains fee amounts for burning CUSD into stable coins. It contains two mappings, one instance variable, three events and five access functions.

The StablecoinWhitelist.sol contract serves as a mapping that indicates whether a token contract is allowed to participate. It contains one mapping, two events and two access functions.

The CarbonDollarStorage.sol contract contains one pre-existing instance each of FeeSheet.sol and StablecoinWhitelist.sol provided at construction time with no mechanism for subsequently changing them. This contract's only usage is to be inherited by MutableCarbonDollarStorage.sol which provides mechanisms for changing each of the two instances noted above.

The MutableCarbonDollarStorage.sol is then inherited by CarbonDollar.sol and CarbonDollar Proxy.sol. Note that the former contract also inherits PermissionedToken.sol which brings along MutablePermissionedTokenStorage.sol.

Proxies

The Carbon contracts make extensive use of proxies, both through specific contracts such as CarbonDollarProxy.sol, PermissionedTokenProxy.sol, RegulatorProxy.sol, DelayedUpgradeabilityProxyMock.soland DelayedUpgradeabilityProxy.sol, as well as through leveraging (inheriting) the zos-lib base contracts. The ultimate objective is to allow upgradable contract logic. Additional documentation can be found on the Zeppelin_OS blog.

Contract Inheritance Diagrams

Contract-to-Whitepaper Coherence

This section discusses how accurately the whitepaper matches the actual contract implementation. Ideally, contracts should aim to implement as closely as possible the various descriptions found in the whitepaper and website. The level of discrepancies found between the whitepaper and actual contract code helps users decide the level of trust they can put into the contracts as implemented.

Carbon’s 21-page whitepaper does a good job introducing the primary goal of price stability, reviewing several use cases, describing the actual price stability mechanism, and the business/competitive landscape. The whitepaper gives a higher-level and longer-term perspective relative to the detailed smart contract implementation.

The contracts themselves currently implement a subset of the whitepaper functionality. Some of this is due to functionality naturally residing outside of the smart contract while other functions such as voting oracle and reward scheme are unimplemented. There is nothing specific in the current smart contracts that suggest operation on Hadera Hashgraph.

Some of the detailed functionality implemented in the smart contracts is not reflected in the whitepaper. This includes proxy, regulator, whitelist and blacklist functionality.

The general discussion section of this audit was developed strictly by examining the raw smart contract code with minimal reference to the whitepaper. The whitepaper and contracts have limited overlap. New Alchemy recommends developing additional documentation to serve as a bridge between the existing whitepaper and smart contract implementation, with a focus on the technical details and operation of the latter. This should include the variety of stakeholder roles, positive and negative usage scenarios, and the supporting regulatory activities. This will greatly improve the ability to identify and resolve logic flaws.

Critical Issues

There were no critical issues found.

Moderate Issues

1. Fixed: Incorrect/Inconsistent Usage of SafeMath

The Carbon smart contracts generally make correct use of the SafeMath library by importing it, attaching it to a datatype (e.g. uint16) via the use for directive, and then using its specific operations (e.g. .add(), .mul(), etc) where needed. However, there are two exceptions.

These contracts utilize SafeMath correctly:

• AllowanceSheet.sol
• BalanceSheet.sol
• DelayedUpgradeabilityProxy.sol
• PermissionedToken.sol

A. The StablecoinWhitelist.sol contract imports the SafeMath library but never uses it. This contract effectively has no math operations. New Alchemy recommends removing the import statement.

B. The FeeSheet.sol contract imports the SafeMath library, attaches it to the uint16 datatype but goes no further. The computeFee() function shown below then uses native (and unsafe) operations.

function computeFee(uint256 amount, uint16 fee) public pure returns (uint256) {
    return (amount * fee) / 1000;
}

An external attacker may call CarbonDollar.sol::computeStablecoinFee() which will then call FeeSheet.sol::computeStablecoinFee() which then finally calls the computeFee() shown above. The amountparameter is passed through this call chain without any validation and may cause an uint256 overflow. As it is unclear how this would result in contract breakage, this issue is marked as moderate instead of critical. New Alchemy recommends fully utilizing SafeMath operations in the computeFee() function.

Re-test v2.0: The Carbon-12 Labs team has restructured the contracts such that the functionality in both StablecoinWhitelist.sol and FeeSheet.sol now resides in CarbonDollarStorage.sol. New Alchemy has inspected this contract and determined that the SafeMath library is now applied correctly and consistently. Thus, this issue has been fully remediated and the associated risk mitigated.

2. Fixed: Pause Should Halt All Significant State-Changing Operations

The PermissionedToken.sol, WhitelistedToken.sol and CarbonDollar.sol contracts all inherit from the OpenZeppelin Pausable base contract. This contract provides an additional state variable ('paused') along with two related modifiers and functions. Together, this functionality is intended to support pausing of all significant state-changing activity, which effectively implements an 'emergency brake' on the contract. The pause functionality is not consistently applied to these Carbon contracts.

The fundamental idea is that if a contract owner suspects malicious activity may be taking place, the owner submits a transaction resulting in the contract ‘pause’ variable being set. All significant state-changing functions should have a whenNotPaused modifier (or equivalent) present which serves as a gatekeeper. This modifier causes the function to revert when the pause variable is set, which effectively pauses contract operation and gives the owner time to carefully consider next steps.

The CarbonDollar.sol contracts are missing whenNotPaused on the following functions:

• listToken() on line 35
• unlistToken() on line 43
• setFee() on line 61
• removeFee() on line 71
• setDefaultFee() on line 81
• mint() on line 109
• convertCarbonDollar() on line 123

The PermissionedToken.sol contracts is missing whenNotPaused on the following function:

• mint() on line 134

The WhitelistedToken.sol contracts are missing whenNotPaused on the following functions:

• mintCUSD() on line 36
• convertWT() on line 54

New Alchemy strongly recommends consistent placement of the whenNotPaused modifier across all state-changing functions, specifically the ten functions listed above.

Note that inheriting from the OpenZeppelin Ownable base contract provides the two functions renounceOwnership()and transferOwnership() which do not implement pause modifiers. While Carbon may consider also adding thewhenNotPaused modifier to these functions, New Alchemy marginally prefers leaving them untouched due to the added complexity of re-implementing and overriding standardized functions.

Re-test v2.0: New Alchemy has inspected the updated CarbonDollar.sol, PermissionedToken.sol and WhitelistedToken.sol contracts noted above and determined that the whenNotPaused modifier has been broadly and correctly applied as recommended. Thus, this issue has been fully remediated and the associated risk mitigated.

3. Fixed: Hardcoded Function Return Values

The optimal way to handle exceptions is a long running subject of debate, even for established languages and frameworks. Solidity in the blockchain context is no exception and adds several unique twists. Typical strategies include:

• Minimal exception handling: aggressively revert when abnormal circumstances are first detected
• Maximal exception handling: revert only as a last resort after extensive handling logic fails

The former case does not utilize return values at all, while the latter case makes extensive use of return values to identify and subsequently handle abnormal circumstances. Each approach is acceptable by itself, but mixing them is an issue.

The PermissionedToken.sol smart contract has a number of function declarations that appear to return a boolean success value, while the actual function body only returns a hardcoded value of true. The adjacent comments do not align with the hardcoded return value. As an example, the mint() function is shown below:

/**
* @notice Allows user to mint if they have the appropriate permissions. User generally
* has to be some sort of centralized authority, e.g. PrimeTrust.
* @dev Should be access-restricted with the 'requiresPermission' modifier when implementing.
* @param _to The address of the receiver
* @param _amount The number of tokens to withdraw
* @return `true` if successful and `false` if unsuccessful
*/
function mint(address _to, uint256 _amount) public requiresPermission returns (bool) {
    return _mint(_to, _amount);
}
    
function _mint(address _to, uint256 _amount) internal userWhitelisted(_to) returns (bool) {
    totalSupply = totalSupply.add(_amount);
    balances.addBalance(_to, _amount);
    emit Mint(_to, _amount);
    emit Transfer(address(0), _to, _amount);
    return true;
}

The comment on line 132 suggesting the return of a success value aligns with the function declarations on line 134 and 138. However, the hardcoded true return value on line 143 runs counter to this -- the function will either return true or revert, never returning false as indicated in the comment. A developer, auditor or user reading code that calls these functions may incorrectly believe they are returning an informative (e.g. success) value. This situation reflects a middle ground of the two approaches that is weaker than either alone.

In some cases, there is risk of one contract calling a non-existent method of another contract which ultimately invokes the fallback function which returns (nothing) normally. These cases may require methods to return hardcoded values in order to differentiate an expected return from a fallback return (which is not allowed to return a value). This does not apply to these.

There are several other additional instances of this as described below.

PermissionedToken.sol
171: function approve(address _spender, uint256 _value) 
     public userNotBlacklisted(_spender) senderNotBlacklisted whenNotPaused returns (bool) {
188: function increaseApproval(address _spender, uint _addedValue) 
     public userNotBlacklisted(_spender) senderNotBlacklisted whenNotPaused returns (bool) {
209: function decreaseApproval(address _spender, uint _subtractedValue) 
     public userNotBlacklisted(_spender) senderNotBlacklisted whenNotPaused returns (bool) {
258: function transfer(address _to, uint256 _amount) public transferConditionsRequired(_to) 
     whenNotPaused returns (bool) {
280: function transferFrom(address _from, address _to, uint256 _amount) 
     public whenNotPaused transferFromConditionsRequired(_from, _to) returns (bool) {
298: function blacklisted() public view requiresPermission returns (bool) {

While the first five functions listed above are part of the ERC-20 standard and thus unchangeable, the last function and mint() have flexibility. New Alchemy recommends Carbon utilize the simpler, minimal exception handling approach which means adopting a policy of reverting as early and aggressively as possible for these two functions. With this strategy, the hardcoded return values can simply be removed from both the function declarations and bodies. The comments should be adjusted to reflect actual code behavior across the contracts.

Re-test v2.0: New Alchemy has inspected the updated PermissionedToken.sol, contract noted above to confirm that the mint() function has been adjusted as recommended, while the five ERC-20 functions correctly remain as-is. Theblacklisted() function serves as a marker device where a hardcoded true is useful for convenience, and so has not been modified. New Alchemy considers this issue fully remediated and the associated risk mitigated.

4. Fixed: Ignored Return Values

The ERC-20 standard^ERC states:

NOTE: Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

The CarbonDollar.sol contract invokes an ERC-20 transfer() function on line 139 without checking the return value. While the contract may expect the target function implementation to match that of the Carbon contracts or even the widely used OpenZeppelin code, New Alchemy nonetheless recommends following the standard as stated above out of an abundance of caution. This can be accomplished by mimicking the code found in SafeERC20^safe as shown below:

require(_token.transfer(_to, _value));

Given the hardcoded return values noted in the prior issue and the ignored return value noted in this issue, New Alchemy recommends careful consideration and consistent application of single strategy for exception handling. Note that lines 138 through 141 utilize mint() and burn() functions that are not part of the ERC-20 standard but might be made consistent by also testing the return values. This paragraph is an informative note only

Re-test v2.0: New Alchemy has inspected the updated CarbonDollar.sol, contract noted above and confirms that the transfer() return value is now correctly checked. Since the mint() and burn() functions have been adjusted as recommended in issue #3 above, the specifics of this issue are no longer applicable to these functions but the general recommendation of a consistent strategy is now evident. New Alchemy considers this issue fully remediated and the associated risk mitigated.

5. Fixed: ERC-20 Double-Spend Attack Vulnerability

The standard ERC-20 interface, which is implemented in PermissionedToken.sol and thus also inherited by CarbonDollar.sol and WhitelistedToken.sol, has a design flaw: if some user Alice wants to change the allowance granted to another user Bob, then Alice checks if Bob has already spent his allowance before issuing the transaction to change Bob's allowance. However, Bob can still spend the original allowance before the transaction changing the allowance is mined, which thus allows Bob to spend both the pre-change and post-change allowances^2. In order to have a high probability of successfully spending the pre-change allowance after the victim has verified that it is not yet spent, the attacker to waits until the transaction to change the allowance is issued, then issues a spend transaction with an unusually high gas price to ensure that the spend transaction is mined before the allowance change. More details on this flaw are available athttps://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/ and https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729.

Due to this flaw, safely using the ERC-20 approve() interface to change allowances requires that:

1. Allowances only change between zero and non-zero
2. Allowances change at most once in a block

These restrictions allow Alice to safely change Bob’s allowance by first setting it to zero, waiting for that transaction to be mined, verifying that Bob didn’t spend its original allowance, and then finally setting the allowance to the new value. Requiring this sequence of operations in approve() would violate the ERC-20 standard, though users can still take this approach even without changes in the approve() implementation.

There are two alternative approaches that contracts can take to mitigate this flaw:

• To make it more convenient to change allowances, New Alchemy recommends providing increaseApproval() and decreaseApproval() functions that add or subtract to the existing allowances rather than overwriting them. This effectively moves the check of whether Bob has spent its allowance to the time that the transaction is mined, removing Bob's ability to double-spend. Clients that are aware of this non-standard interface can use it rather than approve; using approve() remains open to abuse. This approach is currently implemented by the current version of the OpenZeppelin library^3 and Carbon's PermissionedToken.sol contract.
• Only allow approve() to change allowances between zero and non-zero and don't allow multiple changes to a user's allowance in the same block. In order to change Bob's allowance, Alice must first set it to zero, wait until that transaction is mined, verify that the original allowance was not spent, then finally set the allowance to the new value. Requiring this sequence of operations by implementing restrictions in approve() would violate the ERC-20 standard, though users can still take this approach even without changes in the approve() implementation.

As the Carbon smart contract code currently stands, the approve() function does not implement double-spend protection, but both increaseApproval() and decreaseApproval() functions have been implemented. There is remaining risk associated with users transacting with the approve() function.

There are several specific ways to mitigate this risk. New Alchemy recommends disabling the approve() function by default, which will force users to utilize the safer 'increase/decrease' functions. Alternatively, introducing logic intoapprove() that enforces only zero->nonzero and nonzero->zero transitions, would disallow the risky nonzero->nonzero transactions. Both of these alternatives could be supported by a configuration variable set at construction time to provide an opportunity for fallback.

Since these approaches are outside of the ERC-20 standard, they will require user cooperation to work properly. Accordingly, Carbon should provide clear and prominent documentation advising its users on how they should manage other users’ allowances.

Re-test v2.0: New Alchemy has inspected the updated PermissionedToken.sol, contract noted above and confirms that a whenUnlocked modifier has been included on the approve() function as recommended. This modifier is inherited from Lockable.sol and is locked by default. New Alchemy considers this issue fully remediated and the associated risk mitigated.

Minor Issues

6. Fixed: Lack of Two-Phase Ownership Transfer

Effectively all Carbon smart contracts inherit the common Ownable contract from the OpenZeppelin project^oz1. This contract provides for the concept of a single owner, who can unilaterally transfer ownership to a different address. However, if the owner of a contract makes a mistake in entering the address of an intended new owner, then the contract can become irrecoverably unowned. This is analogous to the well-known "Locked Ether" scenario resulting in several million US dollars (over 7000 Ether) irretrievably locked at address 0x0^lock2.

In order to prevent this high-impact scenario, New Alchemy recommends implementing a two-phase ownership transfer. In this model, the original owner designates a new owner but does not actually transfer ownership. The new owner then accepts ownership and completes the transfer. This can be implemented as follows:

contract Ownable {
    address public owner;
    address public newOwner
    
    event OwnershipTransferred(address indexed oldOwner, address indexed newOwner);
    
    function Ownable() public {
        owner = msg.sender;
        newOwner = address(0);
    }
    
    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }
    
    function transferOwnership(address _newOwner) public onlyOwner {
        require(address(0) != _newOwner);
        newOwner = _newOwner;
    }
    
    function acceptOwnership() public {
        require(msg.sender == newOwner);
        OwnershipTransferred(owner, msg.sender);
        owner = msg.sender;
        newOwner = address(0);
    }
}

Re-test v2.0: New Alchemy has inspected the updated project repository to confirm the presence and usage of Ownable.sol in the 'helpers' subdirectory. This contract implements two-phase ownership transfer as recommended. New Alchemy considers this issue fully remediated and the associated risk mitigated.

7. Informational: Lack of Short-Address Attack Protection

Some Ethereum clients may create malformed messages if a user is persuaded to call a method on a contract with an address that is not a full 20 bytes long. In such a “short-address attack”[^saa1], an attacker generates an address whose last byte is 0x00, then sends the first 19 bytes of that address to a victim. When the victim makes a contract method call, it appends the 19-byte address to msg.data followed by a value. Since the high-order byte of the value is almost certainly 0x00, reading 20 bytes from the expected location of the address in msg.data will result in the correct address. However, the value is then left-shifted by one byte, effectively multiplying it by 256 and potentially causing the victim to transfer a much larger number of tokens than intended. msg.data will be one byte shorter than expected, but due to how the EVM works, reads past its end will just return 0x00.

[^saa1]: How to Find $10M Just by Reading the Blockchain https://blog.golemproject.net/how-to-find-10m-by-just-reading-blockchain-6ae9d39fcd95

This attack affects methods where method parameters include an address followed immediately by a value. There are a significant number of these in the Carbon smart contracts, with a summary of each contract, line number and function listed below.

AllowanceSheet.sol
  16: function addAllowance(address _tokenHolder, address _spender, uint256 _value)
  20: function subAllowance(address _tokenHolder, address _spender, uint256 _value)
  24: function setAllowance(address _tokenHolder, address _spender, uint256 _value)

BalanceSheet.sol
  15: function addBalance(address _addr, uint256 _value)
  19: function subBalance(address _addr, uint256 _value)
  23: function setBalance(address _addr, uint256 _value)

CarbonDollar.sol
  61: function setFee(address stablecoin, uint16 _newFee)
 109: function mint(address _to, uint256 _amount)
 123: function convertCarbonDollar(address stablecoin, uint256 _amount)
 
 
FeeSheet.sol
  45: function setFee(address stablecoin, uint16 fee)

PermissionedToken.sol
 134: function mint(address _to, uint256 _amount)
 171: function approve(address _spender, uint256 _value) 
 188: function increaseApproval(address _spender, uint _addedValue) 
 209: function decreaseApproval(address _spender, uint _subtractedValue) 
 231: function destroyBlacklistedTokens(address _who, uint256 _amount)
 258: function transfer(address _to, uint256 _amount)
 280: function transferFrom(address _from, address _to, uint256 _amount) 
 
WhitelistedToken.sol
  36: function mintCUSD(address _to, uint256 _amount)

While the root cause of this flaw is buggy serializers and how the EVM works, it can be easily mitigated in contracts. When called externally, an affected method should verify that msg.data.length is at least the minimum length of the method's expected arguments (for instance, msg.data.length for an external call to PermissionedToken.sol::transfer() should be at least 68: 4 for the hash, 32 for the address (including 12 bytes of padding), and 32 for the value; some clients may add additional padding to the end). This can be implemented in a modifier. External calls can be detected in the following ways:

• Compare the first four bytes of msg.data against the method hash. If they don't match, then the call is internal and no short-address check is necessary.
• Avoid creating public methods that may be subject to short-address attacks; instead create only externalmethods that check for short addresses as described above. public methods can be simulated by having the external methods call private or internal methods that perform the actual operations and that do not check for short-address attacks.

Whether or not it is appropriate for contracts to mitigate the short-address attack is a contentious issue among smart-contract developers. Many, including those behind the OpenZeppelin project, have explicitly chosen not to do so. While it is New Alchemy’s position that there is value in protecting users by incorporating low-cost mitigations into likely target functions, Carbon Money would not stand out from the community if they also choose not to do so.

Re-test v2.0: New Alchemy has discussed this issue with Carbon-12 Labs at length. As noted above, this is a contentious issue among smart contract developers, there are clear community trends towards accepting rather than mitigating this vulnerability, and there are external dependencies such as UI and tools that must be considered. The attack involves transactions signed by ‘msg.sender’. Carbon-12 Labs has chosen to accept this risk at this time and consider future mitigation as their application evolves. New Alchemy considers this an entirely reasonable approach.

8. Fixed: Overcomplex Ownership

A fully deployed set of contracts may involve up to eight owners, potentially a different one for each of the following instances:

• CarbonDollar.sol
• CarbonDollarRegulator.sol
• WhitelistedToken.sol
• WhitelistedTokenRegulator.sol
• FeeSheet.sol
• BalanceSheet.sol
• AllowanceSheet.sol
• ValidatorSheet.sol
• PermissionSheet.sol

The access functions for the latter five ‘sheet’ contracts listed above utilize the onlyOwner modifier. Those access functions are typically called by a top-level contract function protected by a variety of other identity-based modifiers as shown below (from PermissionedToken.sol).

function increaseApproval(address _spender, uint _addedValue) 
public userNotBlacklisted(_spender) senderNotBlacklisted whenNotPaused returns (bool) {
    increaseApprovalAllArgs(_spender, _addedValue, msg.sender);
    return true;
}
function increaseApprovalAllArgs(address _spender, uint256 _addedValue, address _tokenHolder)
internal {
    allowances.addAllowance(_tokenHolder, _spender, _addedValue);
    emit Approval(_tokenHolder, _spender, allowances.allowanceOf(_tokenHolder, _spender));
}

Line 189 above shows usage of the userNotBlackListed and senderNotBlacklisted modifiers. Separately, the mint() function provides another example that utilizes the requiresPermission and userWhitelisted modifiers.

In conjunction with proxy capabilities, there is significant potential for divergence of ownership and brittleness in the access control modifiers. New Alchemy recommends simplifying ownership to a single or much smaller set of addresses.

Re-test v2.0: New Alchemy has discussed this issue with Carbon-12 Labs at length. In the re-tested repository, the ‘Sheet’ contracts have been consolidated into far fewer contracts which reduces the ownership complexity. The presence of several top-level functions along with the proxy strategy put a lower limit on simplification. On balance, New Alchemy considers this issue fully remediated and the associated risk mitigated.

9. Fixed: Redundant Inheritance

Several contracts inherit Ownable directly as well inherit from other contracts that are themselves ownable. The latter makes the former redundant.

These contracts have redundant inheritance:

• Regulator.sol inherits Ownable and MutableRegulatorStorage which itself is Ownable.
• RegulatorProxy.sol inherits Ownable and MutableRegulatorStorage which itself is Ownable.
• DelayedUpgradeabilityProxyMock.sol inherits Ownable and DelayedUpgradeabilityProxy which itself is Ownable
• CarbonDollarProxy.sol inherits Ownable and MutablePermissionedTokenStorage which itself is Ownable

New Alchemy recommends removing the redundant (direct) Ownable inheritance on the four contracts listed above.

Re-test v2.0: New Alchemy reviewed the inheritance structure of the code in the re-tested repository and determined that the redundancies have been removed. New Alchemy considers this issue fully remediated and the associated risk mitigated.

10. Fixed: Opportunities for Additional Input Validation

Insufficient input validation may allow an attacker to pollute the contract’s internal data structures, exploit downstream logic vulnerabilities and/or facilitate simple and easily preventable user mistakes. There are several opportunities for further input validation across the Carbon contracts.

A. The MutableCarbonDollarStorage.sol, MutablePermissionedTokenStorage.sol and MutableRegulatorStorage.solcontracts provide mutability for instances of the lowest-level data structures (e.g. 'sheet*' contracts) . Currently the set functions utilize the OpenZeppelin AddressUtils.isContract() function to confirm the provided addresses are indeed a contract. New Alchemy suggests checking and confirming the ownership field of the provided addresses which will confirm it is a contract and also confirm the expected value of a central parameter at the same time.

B. The PermissionSheet.sol contract provides functions for setting and removing user permissions. These functions could validate the expected original conditions prior to making changes (e.g. require that a permission exists before removing).

C. The Validator.sol contract provides functions for setting and removing validators. These functions could validate the expected original conditions prior to making changes (e.g. require that a validator exists before removing).

New Alchemy realizes there may be external reasons or requirements that prevent incorporating the additional input validation recommended above.

Re-test v2.0: New Alchemy has discussed each of the above items with Carbon-12 Labs at length. Removing storage mutability has resulted in the elimination of the contracts identified in A above. Storage is now constructed via the new operator. The 'permission' and 'validator' items (in B and C) introduce complexity and constraints that conflict with out of scope requirements.

Smart Contract ABI Reference

This section provides the fully-elaborated application binary interface (ABI) for the primary contracts as seen at runtime. The ABI defines how the contracts may be interacted with while running inside the EVM. The tables make for very easy inspection of top-level specifics including:

• The definitive reference of all exposed functions
• Correct input and output function signatures
• Presence of constructors and fallbacks as expected
• Correct state mutability (pure, view, nonpayable, payable)
• Extraneous functions which should be private

CarbonDollar.sol ABI Reference

WhitelistedToken.sol ABI Reference

CarbonDollarRegulator.sol ABI Reference

WhitelistedTokenRegulator.sol ABI Reference

Line-by-Line Comments

This section lists observations on design decisions and code quality made by New Alchemy during the review. They are not known to represent security flaws. Style conventions from the main Solidity documentation^style do not always represent full consensus or uniform common practice.

There is one item that is pervasive across the code base:

• Specific Compiler Pragma. New Alchemy recommends using the pragma annotation to specify a single target compiler version. This helps more clearly elaborate dependencies, isolates the effects of changing tool versions, and insures the code compiles as intended. As of July 2018 the correct statement should target the latest stable compiler: pragma solidity 0.4.24. This issue pertains to every contract in the source set.

A. regulator/Regulator.sol

Line 42

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructor.

Also, the variable names used in the constructor (e.g. p and v) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Lines 72, 108, 136, 165, 182 and 199

New Alchemy recommends differentiating function names from event names by attaching a log prefix to the latter. Functions on the lines referenced above are differentiated from related events only by the case of the first character, which creates potential for confusion and mistakes.

Lines 76, 90, 169, 186 and 203

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line numbers referenced above are internal functions that are interspersed with related public functions. Ideally, these internal functions would be moved after all of the public function declarations.

B. regulator/RegulatorProxy.sol

Line 19

The variable names used in the constructor (e.g. i, p and v) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

C. regulator/carbonDollarRegulator/CarbonDollarRegulator.sol

Line 14

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructor.

Also, the variable names used in the constructor (e.g. p and v) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Lines 20, 29 and 37

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line numbers referenced above are internal functions that should be ideally moved after all of the public function declarations.

D. regulator/dataStorage/MutableRegulatorStorage.sol

Line 18

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructor.

Line 27 and 35

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

E. regulator/dataStorage/PermissionSheet.sol

Line 78

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line number referenced above is the _addPermission() internal function that should be ideally moved after all of the public function declarations.

Line 98

New Alchemy recommends differentiating function names from event names by attaching a log prefix to the latter. The function on the line referenced above is differentiated from its related event only by the case of the first character, which creates potential for confusion and mistakes.

F. regulator/dataStorage/RegulatorStorage.sol

G. regulator/dataStorage/ValidatorSheet.sol

Line 29

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line number referenced above is the _addValidator() internal function that should be ideally moved after all of the public function declarations.

H. regulator/mocks/PermissionSheetMock.sol

I. regulator/mocks/ValidatorSheetMock.sol

J. regulator/whitelistedRegulator/WhitelistedTokenRegulator.sol

Line 14

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructor.

Also, the variable names used in the constructor (e.g. p and v) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Lines 18, 24, 38, 44 and 50

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line numbers referenced above are internal functions that are interspersed with related public functions. Ideally, these internal functions would be moved after all of the public function declarations.

K. tokens/carbonToken/CarbonDollar.sol

Line 27

The variable names used in the constructor (e.g. r, b, a, f and s) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Line 28

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Line 113

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line number referenced above is the _mint() internal function that should be ideally moved after all of the public function declarations.

L. tokens/carbonToken/CarbonDollarProxy.sol

Line 14

The variable names used in the constructor (e.g. i, r, b, a, f and s) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Line 15

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

M. tokens/carbonToken/dataStorage/CarbonDollarStorage.sol

Line 23 and 24

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

N. tokens/carbonToken/dataStorage/FeeSheet.sol

O. tokens/carbonToken/dataStorage/MutableCarbonDollarStorage.sol

Line 22

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Also, the variable names used in the constructor (e.g. f and w) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Line 32

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line number referenced above is the _setStablecoinWhitelist() internal function that should be ideally moved after all of the public function declarations.

Line 34 and 50

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

P. tokens/carbonToken/dataStorage/StablecoinWhitelist.sol

Q. tokens/permissionedToken/PermissionedToken.sol

Lines 4 and 6

This is a duplicate import.

Lines 34 and 35

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Also, the variable names used in the constructor (e.g. r, b and a) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Lines 134, 153 and 258

New Alchemy recommends differentiating function names from event names by attaching a log prefix to the latter. Functions on the lines referenced above are differentiated from related events only by the case of the first character, which creates potential for confusion and mistakes.

Lines 138, 157, 194 and 215

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line numbers referenced above are internal functions that are interspersed with related public functions. Ideally, these internal functions would be moved after all of the public function declarations.

Lines 188 and 209

The _addedValue and _subtractedValue method parameters are declared as uint which defaults to uint256. For clarity, New Alchemy recommends declaring the specific default value of uint256.

R. tokens/permissionedToken/PermissionedTokenProxy.sol

Line 12

The variable names used in the constructor (e.g. i, r, b and a) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

S. tokens/permissionedToken/dataStorage/AllowanceSheet.sol

T. tokens/permissionedToken/dataStorage/BalanceSheet.sol

Line 7

Typo in @title entry (e.g. BalanceSheet not AllowanceSheet).

U. tokens/permissionedToken/dataStorage/MutablePermissionedTokenStorage.sol

Line 19

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Line 31, 39 and 47

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

V. tokens/permissionedToken/dataStorage/PermissionedTokenStorage.sol

Line 26, 27 and 28

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

W. tokens/whitelistedToken/WhitelistedToken.sol

Line 24

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Also, the variable names used in the constructor (e.g. r, b and a) are more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming these variables in a way that their function becomes obvious.

Line 25

Note that the OpenZeppelin AddressUtils.isContract() function will not return true if the tested contract's constructor is currently being executed. Further, core Ethereum developers have indicated that changes to account abstractions may arrive in a future release (Serenity), and this may further impact this function. This note is for future awareness purposes only.

Line 40

Solidity coding style convention prefers a particular order of function declarations: constructor, fallback function (if it exists), external, public, internal then private. Within a grouping, the view and pure functions are placed last. The line number referenced above is the _mintCUSD() internal function that should be ideally moved after all of the public function declarations.

X. upgradeability/DelayedUpgradeabilityProxy.sol

Line 21

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Also, the variable name used in the constructor (e.g. i) is more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming this variable in a way that its function becomes obvious.

Line 34 and 44

Solidity coding style convention prefers the explicit use of block.timestamp over its alias of now to more clearly reflect the dependence upon block mining. Also, note that this value can be manipulated by the block miner by approximately 30 seconds. It does not appear that fundamental contract integrity is dependent upon precise timestamps, but this vulnerability should be kept in mind as the code evolves.

Y. upgradeability/mocks/DelayedUpgradeabilityProxyMock.sol

Line 7

Solidity coding style convention prefers placing the visibility modifier immediately after the parameter declarations. In this instance, the public modifier should be placed before the base constructors.

Also, the variable name used in the constructor (e.g. i) is more cryptic than informative and thus unnecessarily increase cognitive loading. New Alchemy recommends renaming this variable in a way that its function becomes obvious.

Z. upgradeability/mocks/DummyContract.sol

Disclaimer

The audit makes no statements or warranties about utility of the code, safety of the code, compiler or tools stability, suitability of the business model, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug-free status. The audit documentation is for discussion purposes only.

New Alchemy is a leading blockchain strategy and technology group specializing in tokenized solutions for the most innovative companies worldwide. New Alchemy’s Blockchain Security division is a highly trusted name that has assisted clients in securely raising over $500m through custom-tailored solutions, smart contract audits and comprehensive security strategy. Get in touch with us at Hello@NewAlchemy.io