Skip to main content
Dedaub

Archblock Stablecoins & GBPT Upgrade

Smart Contract Security Assessment

Jan 24, 2024

Archblock

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of the Archblock Stablecoins Protocol. Some issues were found with the most important focusing on the upgrade process of PoundToken (previously GBPT) to the new 1GBP implementation and a possible DoS to specific operations of the protocol under certain circumstances.


PROTOCOL DESCRIPTION

Archblock has developed a stablecoin protocol with an integrated Proof of Reserves system. Chainlink feeds can be used to attest to the reserves held by the stablecoin issuer’s banking partners and publish this information on-chain. The protocol is only able to mint an amount of tokens which does not exceed the reserves indicated by the feed at any given time.

The protocol has the ability to create a number of pools, each of which has a certain value, which can be spent to create stablecoin tokens. The value in each pool cannot exceed a given limit, and a threshold is used to determine how many tokens can be minted at once.

Pools are arranged in a chain, and an account with the DEFAULT_ADMIN_ROLE is able to top up the value of the last pool in the chain. Following this, the value of the previous pool in the chain can be refilled up to its limit as long as the right number of accounts with the MINT_RATIFIER_ROLE have approved the refill.

Accounts with the MINTER_ROLE are allowed to create an operation asking for a certain value to be minted into stablecoin tokens. Accounts with the MINT_RATIFIER_ROLE will then be able to approve such an operation. It can then be executed against any pool which will accept to mint tokens given the current number of signatures.

Accounts which own stablecoin tokens are able to redeem them by sending them to an account with an address \<= MAX_REDEMPTION_ADDRESS which also has been assigned the REDEMPTION_ADDRESS_ROLE. Upon such transfer the tokens are immediately burned from the redemption account. Every user interacting with Archblock Stablecoins will be assigned a redemption account which can be used to account for the redemption of the stablecoin tokens into fiat currency.

The protocol has the ability to freeze transfers of the stablecoin to and from certain addresses, by assigning them the FROZEN_ROLE, and can also be paused and unpaused by accounts which have the PAUSER_ROLE.


SETTING & CAVEATS

This audit report mainly covers the code of the at-the-time private trusttoken/contracts-stablecoins repository, found at commit e2c1640062339015616128753e7cd52234c7732a.

We were also requested to review the upgrade process from the old PoundToken, found at src_0_8_4_opt_200_gbpt/BFTokenUpgradable, to the new 1GBP implementation (GBPT_StablecoinWithProxyStorage contract). The storage layout and deployment scripts under the script/ folder were also examined. Some considerations were raised during the auditing of that process which are described in the UPGRADE-PROCESS CONSIDERATIONS section.

As part of the audit, we also reviewed the fixes for the issues included in the report. The fixes were inspected at commit 696a7513861baefeb4dfe37d823a034d1df201a8 and we found that they have been implemented correctly.

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

src_0_8_19_opt_20000/
  • ProxyWrapper.sol
  • Stablecoin.sol
  • library/
    • ApprovalSet.sol
    • MintOperation.sol
    • MintOperationArray.sol
    • MintPool.sol
    • MintPoolArray.sol
    • ProofOfReserve.sol
    • Redemption.sol
src_0_8_19_opt_20000_1gbp/
  • GBPT_StablecoinWithProxyStorage.sol
  • StablecoinWithProxyStorage.sol
  • abstract/
    • GBPT_AccessControlEnumerableUpgradeable.sol
    • GBPT_AccessControlUpgradeable.sol
    • GBPT_ContextUpgradeable.sol
    • GBPT_ERC165Upgradeable.sol
    • GBPT_ERC1967UpgradeUpgradeable.sol
    • GBPT_ERC20Upgradeable.sol
    • GBPT_Initializable.sol
    • GBPT_PausableUpgradeable.sol
    • GBPT_ProxyStorage.sol
    • GBPT_UUPSUpgradeable.sol
  • library/*

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.


PROTOCOL-LEVEL CONSIDERATIONS

P1

Considerations about the future handling of the __gaps in the upgradeable contracts

P1PROTOCOL-LEVEL-CONSIDERATION

Considerations about the future handling of the __gaps in the upgradeable contracts
resolved

It is a common approach for the contracts that are intended to be upgradeable to append a fixed-size uint256[X] array at the bottom of each contract to allow state variable expansion in future versions. However, such additions need special care as some caveats could lead to misaligned storage layouts between the old and the new implementations.

For example, variables less than 32-bytes long can be combined together in the same slot if they fit. So, if, let’s say, 8 new bool variables are added one after another, the __gap[50] should not shrink by 8 slots, but only by 1 and become __gap[49] (see also M1 and A6).

We mention this for future awareness and because several places contain such gap arrays including Structs and the ProxyStorage as well. Namely:

  • src_0_8_19_opt_20000/library/ProofOfReserve.sol:Params
  • src_0_8_19_opt_20000/library/Redemption.sol:Params
  • src_0_8_19_opt_20000_1gbp/library/ProofOfReserve.sol:Params
  • src_0_8_19_opt_20000_1gbp/library/Redemption.sol:Params
  • src_0_8_19_opt_20000_1gbp/abstract/GBPT_ProxyStorage.sol
  • src_0_8_19_opt_20000_1gbp/abstract/ProxyStorage.sol

As a side note, another common convention (assuming OZ’s implementations) is that these gap arrays have an initial fixed-size of 50 words and then the number of slots acquired by the existing storage variables is subtracted. For example, ERC20Upgradeable has 5 state variables that acquire 5 slots in total, hence the uint256[45] gap at the bottom of the contract. This, however, doesn’t seem to be exactly followed in the two structs listed above which declare uint256[50] __gap arrays, but they also have several fields declared. We mention this just for completeness and consistency in case such a standard is desired to be followed throughout the codebase.

P2

Pool refill can be proven expensive in the long term

P2PROTOCOL-LEVEL-CONSIDERATION

Pool refill can be proven expensive in the long term
dismissed

Dismissed

The team explained that they anticipate each pool to grow by at least 1-3 orders of magnitude compared to the previous one, and that they only plan to create ~3 pools in total and they do not desire to enforce these as constraints, since operationally there may be edge cases where they would like to break these assumptions.

The idea of the MintPools is to have several of them with the setting that each next Pool in the chain should have the same or higher limit, threshold and signatures required for operations than the previous one.

All the Pools (except the last one) are allowed to be refilled only by the Pool right next to them in the chain. This means that refilling the Pools that are deeper in the chain would require approvals and execute several refills to reach the first Pools of the chain and have them also refilled first. Assuming the trivial chain with n Pools and all having the same setup, then the complexity of refilling all the Pools in the chain would be O(n2)O(n^2) and refilling the leftmost one O(n)O(n). However, this can be reduced a bit by having large Pools that can provide several refills for the Pools on the left, but this only saves the multiple refills of its own. The approvals should be given for each refill operation every time.



UPGRADE-PROCESS CONSIDERATIONS

U1

Missing validations of the executed operations

U1UPGRADE-PROCESS-CONSIDERATION

Missing validations of the executed operations
resolved

The script 04_Upgrade.s.sol is intended to be used for upgrading to the new implementation of the 1GBP token. This script, however, is only changing the underlying implementation and calls the new initializeV2 function. Nevertheless, several validation checks could be performed after this process to ensure that everything has gone as expected and that no downtime will occur. More specifically:

  • You could add checks to ensure that all the old values can still be read and written after the upgrade, that the same as the old values are retrieved and that basic interactions with the protocol are possible and result in the expected outcome. In any different result, the process should revert to the previous state.

  • The specifications of the new implementation suggest that several important roles should be assigned to different accounts. Roles like the default admin, the mint ratifiers and the restriction of the previously blacklisted accounts should be given and performed, correspondingly, as soon as possible.

  • Checks ensuring the correct assignment of these roles could also be added

U2

Convoluted and conflicting logic in the upgrade scripts

U2UPGRADE-PROCESS-CONSIDERATION

Convoluted and conflicting logic in the upgrade scripts
resolved

The codebase contains upgrade scripts that are intended to be used for either new deployments on testnets, local devnets or chains other than Ethereum.

  • 01_Deployments.s.sol
  • 02_Verification.s.sol
  • 03_Initialization.s.sol
  • 04_Upgrade.s.sol

However, assuming the desired upgrade process of the old PoundToken to the new 1GBP, these scripts contain conflicting parts and operations that should not be expected to run throughout the process.

The team informed us that only the 04 is intended to be used for the upgrade operation, but the actual deployment of the GBPT_Stablecoin contract only happens inside the 01 script which also contains several more deployments including one for the general Stablecoin and more get or deploy operations over several other instances of proxies and the old PoundToken too.

We suggest revisiting the whole upgrading flow implemented in the scripts and having a separate workflow for the upgrade and the test deployments so that overlapping parts do not introduce conflicts that could lead to unexpected results.

U3

Use of Enums in upgradeable contracts needs attention

U3UPGRADE-PROCESS-CONSIDERATION

Use of Enums in upgradeable contracts needs attention
acknowledged

The use of Enums in contracts that are expected to be upgradeable has some caveats that should be considered carefully for future updates.

If a defined Enum type is used in multiple different contracts, which might also communicate to each other, then any addition of fields to the Enum declaration requires redeployment or upgrade for ALL the contracts that previously used that Enum type. There have been cases where this was proven expensive as a lot of upgrades should happen or are impossible when a non-upgradeable contract uses them, has no migration logic implemented and holds sensitive data that cannot be transferred to a new deployment.

In this codebase, the Enum type was introduced in the MintOperations libraries only which are used by the main Stablecoin contracts. We raise this for future awareness, in case new separate contracts use this library and new Enum values are added at some point. In such a case, Stablecoins and all other contracts should be upgraded to include the new Enum version.



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

M1

Misaligned storage layouts

M1MEDIUM

Misaligned storage layouts
resolved

Resolved

The storage layout was properly aligned with the old storage at commit: b11abb3

The GBPT_Stablecoin contract is expected to replace the old implementation of the PoundToken that can be found at 0x86B4dBE5D203e634a12364C0e428fa242A3FbA98 (proxy) and 0xf08421CE7c9e57aCBd183A92e295809Fcb6325c3 (old implementation). Due to the structure of the old contract, the team decided to flatten all the previous and newly imported libraries (like OpenZeppelin’s) and use local copies of all these imports. Namely, the libraries were copied from OpenZeppelin at version v4.9.5 and from Chainlink at version v2.7.2.

To preserve the storage layout intact after the upgrade to the new implementation, a new GBPT_ProxyStorage.sol contract was introduced keeping all the state variables declared in all the usually imported libraries and the local variables of the Stablecoin itself. This contract is being inherited by the local GBPT_Initializable.sol which in turn is the base-most library in the inheritance chain.

However, a misalignment between the old and the new layouts seems to have happened during that merging and the storage was pushed down by 47 slots.

  • src_0_8_4_opt_200_gbpt/BFTokenUpgradeable.sol:BFTokenUpgradeable
    | Name           | Type                                            | Slot |
    |----------------|-------------------------------------------------|------|
    | _initialized | bool | 0 |
    | _initializing | bool | 0 |
    | __gap (CX Upg) | uint256[50] | 1 |
    | _balances | mapping(address => uint256) | 51 |
    | _allowances | mapping(address => mapping(address => uint256)) | 52 |
    | _totalSupply | uint256 | 53 |
    | _name | string | 54 |
    | _symbol | string | 55 |
    | __gap (ERC20) | uint256[45] | 56 |
    | __gap (ERC20Bu)| uint256[50] | 101 |
    | config | contract IConfigV1 | 151 |
    | __gap (BVU) | uint256[50] | 152 |
    | IssueToAddress | address | 202 |
    | paused | bool | 202 |
    | blacklisted | mapping(address => bool) | 203 |
  • src_0_8_19_opt_20000_1gbp/GBPT_StablecoinWithProxyStorage.sol:Stablecoin
    | Name                  | Type                                                           | Slot |
    |-----------------------|----------------------------------------------------------------|------|
    | _initialized | uint8 | 0 |
    | _initializing | bool | 0 |
    | __gap0 (CX Upg) | uint256[50] | 1 |
    | _balances | mapping(address => uint256) | 51 |
    | _allowances | mapping(address => mapping(address => uint256)) | 52 |
    | _totalSupply | uint256 | 53 |
    | _name | string | 54 |
    | _symbol | string | 55 |
    | __gap1 (ERC20) | uint256[45] | 56 |
    | __filler0 | uint256[148] | 101 |
    | __filler1 (Issue2Addr)| address | 249 |
    | _paused | bool | 249 |
    | __filler2 (blcklisted)| mapping(address => bool) | 250 |
    | __gap2 | uint256[50] | 251 |
    | _roles | mapping(bytes32 => struct ProxyStorage.RoleData) | 301 |
    | __gap3 | uint256[49] | 302 |
    | _roleMembers | mapping(bytes32 => struct EnumerableSetUpgradeable.AddressSet) | 351 |
    | __gap4 | uint256[49] | 352 |
    | __gap5 | uint256[50] | 401 |
    | __gap6 | uint256[50] | 451 |
    | __gap7 | uint256[49] | 501 |
    | _mintOperations | struct MintOperationArrays.Array | 550 |
    | _mintPools | struct MintPoolArrays.Array | 551 |
    | _proofOfReserveParams | struct ProofOfReserve.Params | 552 |
    | _redemptionParams | struct Redemption.Params | 604 |

The __filler0 has been declared with a size of 148 words and it was intended to contain the ERC20BurnableUpgradeable and the BaseVerifyUpgradeableV1 gaps (each of 50-word length) and the IConfigV1 config which is of type address (20-bytes -> 1 slot) and they all total in 101 slots that should be covered by the __filler0.

However, these extra 47 slots push the deprecated IssueToAddress (__filler1), the deprecated blacklisted (__filler2) and the _paused variables down. It seems that it causes no harm to the protocol as 2 out of 3 are not going to be used at all in the new version and the _paused variable will be pushed to a new slot acquiring the default value (false), but it is certainly something that is unintentional and should be considered carefully and tested thoroughly to ensure the correctness of the two versions and their layouts.

M2

The initialization of the new 1GBP implementation fails to configure the new variables correctly

M2MEDIUM

The initialization of the new 1GBP implementation fails to configure the new variables correctly
resolved

Resolved

The team wants to make the upgrade process as easy as possible for the GBPT owner they moved more logic to the contract initialization, so the whole process can be done by calling upgradeToAndCall on the proxy. Namely, several blacklisting and role-granting operations were added. The changes can be found at commit: 28d5372.

The GBPT_StablecoinWithProxyStorage contract is meant to replace the old implementation of the PoundToken. However, it has been copied from the general Stablecoin contract implementation which introduced some bugs that could lead to misconfigured implementation with no option to recover.

The initialize function has the initializer modifier in place, but since this is an upgrade of an existing old already initialized contract the modifier would not allow the initialize function to run. For that reason, the initializeV2 was introduced with the reinitializer(2) modifier to allow a second initialization after upgrading. However, the initialize function contains logic that is important for the new implementation. More specifically, the important calls that are missing from the new initializer are the following:

  • _setRoleAdmin(REDEMPTION_ADDRESS_ROLE, REDEMPTION_ADMIN_ROLE);
  • _proofOfReserveParams.setDecimals(decimals());

The _setRoleAdmin might be possible to be done after the upgrade, but the decimal set to the PoR system cannot be done later as the setDecimals function is internal and no public wrapper seems to exist. As a result, the implementation will be in a state where the PoR system would not be able to operate at all and the Stablecoin as a consequence.

We suggest moving all the important calls to the new initializeV2 to ensure that all the values will be set correctly upon initialization so that the protocol operates as expected.

M3

DoS could happen to the refill and the minting operations when revoking the MINT_RATIFIER_ROLE

M3MEDIUM

DoS could happen to the refill and the minting operations when revoking the MINT_RATIFIER_ROLE
acknowledged

Acknowledged

The team updated the documentation (at commit 696a751) to mention the possible DoS and provide a workaround in case the situation occurs. The workaround would be to re-grant the MINT_RATIFIER_ROLE to the addresses from where it was previously revoked, execute the finalization and then revoke the role from that address again, all in the same transaction to prevent those addresses from interacting as MINT_RATIFIER_ROLE again with the contract.

In the Stablecoin contract, MintPools are used that allow certain amounts of tokens to be minted when such an operation gets approved. Once the Pools are emptied they need to be refilled in order to process future mintings. The accounts that can approve a refill or a minting operation are the ones with the MINT_RATIFIER_ROLE. When sufficient approvals have been submitted anyone can execute the approved operation.

However, the functions that finalize these tasks perform an additional check to examine whether the accounts that have sent their approvals, still have the MINT_RATIFIER_ROLE or not. While this seems to be a good security practice since an account with that role could have approved an operation but have the role revoked before the finalization, it may also cause a DoS to the minting and refilling system (when certain conditions are met).

More specifically, consider the following example:

  • Assume that a Pool needs a refill and the maximum number of approvals (MAX_COUNT=8) are required for that operation

  • Let’s also assume that there are > 8 accounts with the MINT_RATIFIER_ROLE

  • 8 of them have approved the refill operation, but before the finalization, 1 of them gets his role revoked

  • This essentially means that the finalization function will fail to execute the refill as it counts how many of the accounts that have approved have the role at the time of finalization (which will be found to be 7)

  • There also doesn’t seem to be a direct way to remove that approval from the set of the submitted approvals

  • Finally, only MAX_COUNT approvals are allowed to be sent for each operation which means that no one else can vote for the finalization of the operation as 8 people have voted already, but only 7 are counted as valid. As a result, this would lead to a DoS to the system in the long term when all the Pools, before the blocked one, get emptied and require this one to be refilled first for them to be also refilled.

More generally, for this issue to manifest the following conditions have to be met:

  • #-of-revoked-roles > (MAX_COUNT - #-of-approval-sigs-submitted)
  • This, however, can happen when:
    #-of-approval-sigs-submitted > MAX_COUNT / 2

While these constraints may indicate that this scenario could be not very likely to occur, it is still possible to happen and cause a DoS to the system. Workarounds exist with the current code, but they could be found to be error-prone, complex and more expensive to execute. For example:

  • You could re-grant the MINT_RATIFIER_ROLE to the account from which it was revoked, run the finalization function and then revoke the role again. All in the same transaction to prevent the account from interacting with the protocol as MINT_RATIFIER_ROLE.

  • The admin can start deleting the Pools up to the one that causes the DoS and then recreate new ones. This, however, could be proven excessively expensive in case the blocked Pool is deep in the Pool chain.

A possible solution to all that could be to implement a new function for the DEFAULT_ADMIN_ROLE only, from which the admin would be able to delete a specific approval from a Pool, only if that account does not have the MINT_RATIFIER_ROLE anymore. This does not seem to increase the trust in the system and it also does not pose further centralization risks since the admin would not be able to remove valid approvals from the Pools and hamper the normal execution.



LOW SEVERITY

L1

Deprecated initialization practice

L1LOW

Deprecated initialization practice
resolved

In the upgradeable contract logic, it is a good practice to always initialize or disable all the initializations for the implementation contracts. For this purpose, libraries like OpenZeppelin’s Initializable.sol, implement ways to prevent the initializations.

Before version v4.6.0 the usual way was to add the initializer modifier to the constructor() and the initialize() function of the implementation contract. This would effectively prevent the initializer function from running as the constructor would have set the initialized variable to true upon construction.

From version v4.6.0 onwards, however, OZ introduced the reinitialization functionality which allows a later version to reinitialize the state if needed by using an only incrementing index and the reinitializer modifier. For that reason, the _disableInitializers() function was also introduced which should be invoked inside the constructor to disable all initializers from running in the implementation contract.

However, although a version > v4.6.0 is used in all the Stablecoin contracts the old style of disabling the initializers has been kept. These settings make an issue manifest in the GBPT_StablecoinWithProxyStorage contract which is intended to replace the old implementation of the BFTokenUpgradeable.

(GBPT_)Stablecoin::constructor():71
constructor() initializer {}

function initialize(
string memory name_, string memory symbol_
) public initializer { ... }

function initializeV2() public reinitializer(2) {
_grantRole(DEFAULT_ADMIN_ROLE, address(0x123));
__ERC20_init("Pound Token", "1GBP");
}

A new initializer has been added to rename the token and also give the DEFAULT_ADMIN_ROLE to a new hardcoded address. As a result, the initializeV2() function can be called in the implementation as the initializer only sets the _initialized variable to 1 and not to the maximum uint256.

Even though this new initializer seems to cause no harm in case a malicious user executes it in the implementation contract directly, it is suggested to remove the initializer from the constructor and call the _disableInitializers() function instead to ensure that future upgrades would also be guarded in case they contain more important configurations or components.

This change should happen to all the Stablecoin instances of the codebase. Namely:

  • src_0_8_19_opt_20000/
  • Stablecoin.sol:Stablecoin:72
  • src_0_8_19_opt_20000_1gbp/
  • GBPT_StablecoinWithProxyStorage.sol:Stablecoin:71
  • StablecoinWithProxyStorage.sol:Stablecoin:71

L2

Initializers of inherited contracts are not called

L2LOW

Initializers of inherited contracts are not called
resolved

The functions __UUPSUpgradeable_init of the UUPSUpgradeable contract and the __Pausable_init of the PausableUpgradeable are not called by the initialize functions of the following contracts that inherit/use UUPSUpgradeable:

  • src_0_8_19_opt_20000/
  • Stablecoin.sol:Stablecoin:80
  • src_0_8_19_opt_20000_1gbp/
  • GBPT_StablecoinWithProxyStorage.sol:Stablecoin:73
  • StablecoinWithProxyStorage.sol:Stablecoin:73 __UUPSUpgradeable_init may have an empty implementation at the moment but that could change in a future version of OpenZeppelin’s code, which could be used by an upgraded version of the aforementioned contracts. If such a change was to go unnoticed or was not followed by the introduction of a call to the initializers, this omission could lead to a serious bug.

__Pausable_init on the other hand is not empty, as it contains the initialization of the _paused variable. Even though the default initialization of the variables would give the value false to _paused, similarly to __UUPSUpgradeable_init, we suggest always adding all the initializers of the inherited contracts to be sure about the correct functioning of the protocol for the current and the future versions.

L3

Non-ERC20 compliant tokens are not able to be reclaimed

L3LOW

Non-ERC20 compliant tokens are not able to be reclaimed
resolved

The Stablecoin contracts implement functionality that allows the admins to reclaim any amount of Ether or ERC20 tokens sent directly to the Stablecoin’s address. However, there are ERC20-like tokens that do not completely follow the ERC20 standards and do not return a high-level value after transferring the tokens. A well-known token in that category is USDT.

As of Solidity version 0.4.22 and later, the produced code checks the size of the return value after an external call and reverts the transaction in case the return data is shorter than expected. As a result, tokens that do not return a boolean value on transfer but the IERC20.transfer() interface is used, won’t be possible to be reclaimed by the admin of the Stablecoin.

For that reason, we suggest using the safeTransfer alternative wrapper which handles such tokens and ensure that they would be able to be reclaimed if needed.

(GBPT_)Stablecoin::reclaimToken():306
function reclaimToken(
IERC20Upgradeable token
) external onlyRole(DEFAULT_ADMIN_ROLE) {
uint256 balance = token.balanceOf(address(this));
// Dedaub: Non-ERC20 compliant tokens (like USDT) won't be able to be
// reclaimed. Consider using the safeTransfer instead
token.transfer(msg.sender, balance);
emit ReclaimToken(token, msg.sender, balance);
}


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

Centralization issues with respect to DEFAULT_ADMIN_ROLE and to the MINTER_ROLE

N1CENTRALIZATION

Centralization issues with respect to DEFAULT_ADMIN_ROLE and to the MINTER_ROLE
acknowledged

The account with the DEFAULT_ADMIN_ROLE, has a number of privileges with respect to other accounts in the system. Most notably, he can:

  • Transfer tokens and receive tokens from accounts which have been frozen, and may do so even when the protocol is paused

  • Burn the tokens of any account

  • Change the Chainlink feed attesting to the reserves available to the protocol

  • Add and remove pools, and adjust any limits, thresholds and number of signatures related to the pools.

The team has indicated that these features are necessary to ensure a fast reaction time to problems, as well as for dealing with sanctioned addresses. The account will be controlled by a multisig as a risk mitigation measure.

In addition to the above, the account with the DEFAULT_ADMIN_ROLE can create pools which support minting by an account with the MINTER_ROLE without approval from any account with the MINT_RATIFIER_ROLE. The account with the MINTER_ROLE will be used by an off-chain component controlled by the team, while addresses with the MINT_RATIFIER_ROLE will be controlled by members of the team with a hardware wallet. The team has indicated that accounts with the MINTER_ROLE will be able to instantly mint small amounts of tokens, but that this will be controlled by setting appropriate pool parameters so that these pools are only used for this purpose.



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

Missing check when finalizing a refill from a Pool

A1ADVISORY

Missing check when finalizing a refill from a Pool
dismissed

Dismissed

This constraint is not necessary at the abstraction level of the mint pool, which should not care what position it's located. The team deliberately allowed mint pools to be refilled from any other mint pool, but only at a higher level enforced that they are refilled from the next one.

In the MintPool contracts, the finalizeRefillFromPool function checks whether the Pool to be refilled is the same as the Pool from which it will be refilled to avoid double spending and issuing “free” tokens.

However, the specifications suggested that a refill operation can only happen from a larger to a smaller Pool. This means that a refill should not also be possible to happen from a previous Pool in the chain than the one that is being refilled.

The code enforces the refill to happen from the next Pool only, but even having that, the code still checks about the equality mentioned above. So, we expect that a check to ensure that the current Pool is being refilled by a Pool after it in the chain could be also added in case you think that such a call could be introduced in the future or user-controlled Pool indexes could be used at some point later.

A2

View functions may show stale approvals

A2ADVISORY

View functions may show stale approvals
mitigated

Mitigated

The functions mentioned in the issue were renamed to viewUnfiltered...() to make clear what to be expected from their return values.

In the Stablecoin contract, the viewMintPoolRefillApproval() function shows all approvals which were granted to a given pool, even those which no longer have the MINT_RATIFIER_ROLE. It is thus possible that this view function may report stale approvals. Similarly, for the viewMintOperationRatifierApproval() function.

The following contracts are affected:

  • src_0_8_19_opt_20000/
  • Stablecoin.sol
  • src_0_8_19_opt_20000_1gbp/
  • GBPT_StablecoinWithProxyStorage.sol
  • StablecoinWithProxyStorage.sol

A3

Misleading view function names

A3ADVISORY

Misleading view function names
resolved

In the Stablecoin contract, the viewRedemption() function returns the minimum redemption amount, but this is not clear to a caller from its name. Consider changing this function name to viewMinimumRedemptionAmount() or similar.

The following contracts are affected:

  • src_0_8_19_opt_20000/
  • Stablecoin.sol
  • src_0_8_19_opt_20000_1gbp/
  • GBPT_StablecoinWithProxyStorage.sol
  • StablecoinWithProxyStorage.sol

Moreover in the MintPool contract, the refillApprovals() function returns a count of the refill approvals which have been signed by an address that has the MINT_RATIFIER_ROLE, rather than the actual refill approvals. Consider changing this function name to viewRefillApprovalCount() or similar.

A4

Different assumptions are taken compared to an older design when unpausing the protocol

A4ADVISORY

Different assumptions are taken compared to an older design when unpausing the protocol
resolved

The Stablecoin contracts can be (un)paused by the PAUSER_ROLE. However, in our previous audit, the implementation followed a different pattern that allowed the PAUSER_ROLE only to pause the protocol and have the DEFAULT_ADMIN_ROLE restart it.

The rationale behind this logic was that in the event of an incident, the pausing of the protocol should be quick enough as a preliminary measure, but the unpausing should be slow and performed with diligence only after the situation has been resolved.

In the current implementation, however, the PAUSER_ROLE controls both the pausing and the unpausing of the Stablecoin. While this is a usual approach for a wide range of Pausable protocols, it might not be the desired one considering the previous design.

A5

assert() is used throughout the entire codebase

A5ADVISORY

assert() is used throughout the entire codebase
info

There are several places in the Stablecoin contracts where assert() calls are used to check the values and the correctness of specific variables. However, most of these checks seem to be trivial or redundant.

MintPool::finalizeRefillFromPool():119
// Dedaub: value <= limit is enforced by other checks in the code
// performed when updating their values
assert(self._value <= self._limit);

uint256 refillAmount = self._limit - self._value;
other.spend(refillAmount);
self._value += refillAmount;
// Dedaub: This is a trivial check (for production) as the result comes
// from the above addition which is bounded
assert(self._value == self._limit);

There is another caveat with assert() which is that it consumes all the remaining gas in case of a failed assertion. The require() statements, on the contrary, refund all the remaining gas. We assume that they have been put there for testing purposes, but we would suggest reconsidering about them before production, removing those that are trivial and unnecessary, as they consume more gas, and converting to require() statements the ones that you would like to keep.

A6

Possible gas optimizations #1

A6ADVISORY

Possible gas optimizations #1
info

In the ProofOfReserve contract, the checkMint function is called every time a minting operation is being finalized to ensure that no more than the total fiat reserves held in escrowed accounts can be minted. However, there are a couple of changes that can be made to optimize that check and save some gas. More precisely:

ProofOfReserve::checkMint():42
function checkMint(
ProofOfReserve.Params storage self,
uint256 amount,
uint256 totalSupply
) internal view {
if (!self._enabled || address(self._feed) == address(0)) {
return;
}
if (self._feed.decimals() != self._decimals) {
revert ProofOfReserveFeedDecimalsDoNotMatch(
self._feed.decimals(), self._decimals);
}
...
}
  • The address(self._feed) == address(0) check of the 1st if statement above seems to be redundant as when the PoR is disabled, this check will not be evaluated and when it is enabled this check will always evaluate to false as if the self._feed was 0 then the self._enabled would also be false as it is enforced by the setter functions.
ProofOfReserve::setEnabled():66
function setEnabled(
ProofOfReserve.Params storage self,
bool enabled_
) internal {
// Dedaub: The PoR system cannot be enabled when the _feed
// has not been set
if (enabled_) {
if (address(self._feed) == address(0)) {
revert ProofOfReserveFeedIsAddressZero();
}
...
}
self._enabled = enabled_;
}
ProofOfReserve::setFeed():78
function setFeed(
ProofOfReserve.Params storage self,
AggregatorV3Interface feed_
) internal {
self._feed = feed_;
// Dedaub: This enforces the PoR system to be disabled when the
// feed_ == address(0)
if (address(feed_) == address(0)) {
self.setEnabled(false);
}
}
  • The check self._feed.decimals() != self._decimals of the 2nd if statement seems to also be trivial (in the context of checkMint) and can be extracted out to the setter functions. The calls to checkMint are expected to be of a much higher rate than the calls to the feed and the decimals setters. Thus, you could add cross checks in these setters to ensure that this condition holds every time one of them changes and remove it from the checkMint to save 3 SLOADs per call. More specifically:

  • You could add a decimal check in the setFeed function if the self._decimals have already been set and ensure that the feed which is being added has the same decimals as well AND if the self._decimals haven’t been set yet, then set the self._decimals value based on the decimals of the provided feed.

  • You also have to add a similar check in setDecimals. If the self._feed has been set then compare the new decimals_ value against the feed’s decimals AND if the self._feed has not been set yet, then just assign the new value to the self._decimals as usual.

A7

Possible gas optimizations #2

A7ADVISORY

Possible gas optimizations #2
resolved

Resolved

Fixes provided at commit: b11abb3

Taking into account the issue described in M1 and as part of the introduction of the new custom ProxyStorage, there seems to be a possibility for a gas optimization.

More specifically, the bool _paused variable was used to be ordered right after the IssueToAddress, which is of type address (20-bytes). As a result, and since they both fit in a single slot, they were used to be packed together (see the 1st table of M1 at slot 202).

However, the new storage layout has deprecated and replaced the IssueToAddress variable with the general __filler1, but the two variables will continue to be packed together. The difference now is that only the _paused variable will be widely used (actually on every call that involves the _beforeTokenTransfer hook).

The operations to pack and unpack multiple variables into the same slot consume more gas compared to simply reading the slot atomically as a single variable. This can be proven beneficial if the packed variables are consumed by the protocol together each time the slot is read, but it could become more expensive if only one of the packed values is used, which is the case here with _paused (ref. Layout of State Variables in Storage)

Having said that and also considering the importance and the role of the _paused variable, we think that you could also deprecate the old paused variable and move the new _paused one down to the PausableUpgradeable section of the GBPT_ProxyStorage contract (similarly for the ProxyStorage).

A8

Possible gas optimizations #3

A8ADVISORY

Possible gas optimizations #3
info

In the MintPoolArray contract, the setThreshold and setLimit functions could be simplified to save some gas. For example, they check the new thresholds and limits against both the values of the thresholds and the limits of the previous in the chain Pools.

However, the condition threshold <= limit is enforced for every Pool by the MintPool::setThreshold and MintPool::setLimit functions.

src_0_8_19_opt_20000/library/MintPool::setThreshold():77src_0_8_19_opt_20000_1gbp/library/MintPool::setThreshold():77
function setThreshold(
MintPools.Pool storage self,
uint256 threshold_
) internal {
if (threshold_ > self.limit()) {
revert ThresholdExceedsLimit(threshold_, self.limit());
}
self._threshold = threshold_;
}
src_0_8_19_opt_20000/library/MintPool::setLimit():84src_0_8_19_opt_20000_1gbp/library/MintPool::setLimit():84
function setLimit(
MintPools.Pool storage self,
uint256 limit_
) internal {
if (limit_ < self.threshold()) {
revert ThresholdExceedsLimit(self.threshold(), limit_);
}
self._limit = limit_;
self._value = MathUpgradeable.min(self._value, limit_);
}

As a result, in MintPoolArray::setThreshold and in MintPoolArray::setLimit you can remove one of the two checks as the other is abided. More specifically:

  • You can remove the threshold < self.at(poolIndex.prev()).threshold() from the MintPoolArray::setThreshold
src_0_8_19_opt_20000/library/MintPoolArray::setThreshold():80 \src_0_8_19_opt_20000_1gbp/library/MintPoolArray::setThreshold():80
function setThreshold(
MintPoolArrays.Array storage self,
PoolIndex poolIndex,
uint256 threshold
) internal {
// Dedaub: This condition is covered by the next one as
// threshold <= limit for all the Pools. We care about the
// threshold of the current Pool not being less than the
// limit AND the threshold of the previous one. So,
// checking that it is above the limit of the previous
// we ensure that it is also above its threshold.
if (poolIndex > PoolIndices.ZERO &&
threshold < self.at(poolIndex.prev()).threshold()) {
revert ...;
}
if (poolIndex > PoolIndices.ZERO &&
threshold < self.at(poolIndex.prev()).limit()) {
revert ...;
}
...
}
  • You can remove the limit > self.at(poolIndex.next()).limit() from the MintPoolArray::setLimit
src_0_8_19_opt_20000/library/MintPoolArray::setLimit():96 \src_0_8_19_opt_20000_1gbp/library/MintPoolArray::setLimit():96
function setLimit(
MintPoolArrays.Array storage self,
PoolIndex poolIndex,
uint256 limit
) internal {
...
// Dedaub: This condition is covered by the next one as
// threshold <= limit for all the Pools. We care about the
// limit of the current Pool not being greater than the
// limit AND the threshold of the next one. So, checking
// that it is below the threshold of the next we ensure
// that it is also below its limit.
if (poolIndex.next() < self.length() &&
limit > self.at(poolIndex.next()).limit()) {
revert ...;
}
if (poolIndex.next() < self.length() &&
limit > self.at(poolIndex.next()).threshold()) {
revert ...;
}
...
}

A9

Missing code from the copied libraries

A9ADVISORY

Missing code from the copied libraries
resolved

The GBPT_* contracts in the src_0_8_19_opt_20000_1gbp/abstract directory are supposed to be directly copied from OpenZeppelin’s upgradeable repo at v4.9.5. However, the GBPT_ContextUpgradeable seems to have been copied from an older version since the _contextSuffixLength function, which was added as part of the fix for the recent vulnerability related to the combination of the Multicall and the ERC2771Context modules, is missing from the local copy. This function seems to not be crucial for the current implementation, but we report this as an inconsistency in the assumptions taken for the copied code and to be aware of similar cases in the future.

A10

Compiler bugs

A10ADVISORY

Compiler bugs
info

The code is compiled with Solidity 0.8.19. Version 0.8.19, 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