Archblock Stablecoins & GBPT Upgrade

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.

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(n^2)$ and refilling the leftmost one $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.

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

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.

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.

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.

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.

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.

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.

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

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.

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.

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

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.

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.

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

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.

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.

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.

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

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:

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.

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

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.

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

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.

function setThreshold(
    MintPools.Pool storage self,
    uint256 threshold_
) internal {
    if (threshold_ > self.limit()) {
        revert ThresholdExceedsLimit(threshold_, self.limit());
    }
    self._threshold = threshold_;
}

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

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

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 ...;
    }
    ...
}

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.

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.