Centrifuge v3.1 Fix Review

BatchRequestManager.setEpochIds implements functionality for a BatchRequestManager ward to override values in epochId mapping during a migration from V3's ShareClassManager to V3.1's BatchRequestManager.

The method could benefit from an event emission in order for off-chain components to be able to track calls to the highlighted method.

Informational.

Define an EpochIdModified(PoolId pool) event and log it within the highlighted method:

@@ -94,9 +94,12 @@ contract BatchRequestManager is Auth, BatchedMulticall, IBatchRequestManager {
         _;
     }
 
+    event EpochIdModified(PoolId poolId, ShareClassId scId, AssetId assetId, EpochId epochIdData);
+
     /// @dev used only for migrations
     function setEpochIds(PoolId poolId, ShareClassId scId, AssetId assetId, EpochId memory epochIdData) external auth {
         epochId[poolId][scId][assetId] = epochIdData;
+        emit EpochIdModified(poolId, scId, assetId, epochIdData);
     }

Fixed by 0eb5d40

ISpoke.request now accepts an extraGasLimit parameter in order for callers to provide an additional gas stipend for cross-chain execution.

The highlighted method's natspec documentation is missing a description for the extraGasLimit parameter.

Informational.

Add a description for the extraGasLimit to the highlighted method's documentation.

Fixed by 740efd3.

The SubsidyManager contract emits a WithdrawSubsidy event inside withdraw() but not inside withdrawAll().

Since both functions have similar behavior, with the main difference being that withdrawAll() withdraws the entire available balance, it seems expected that it would also emit the WithdrawSubsidy event.

Informational.

Emit WithdrawSubsidy event inside withdrawAll().

function withdrawAll(PoolId poolId, address to) external auth returns (address, uint256) {
...
   refund.withdrawFunds(to, amount);
+  emit WithdrawSubsidy(poolId, to, amount);

   return (address(refund), amount);
}

Fixed by 7934e82.

The vault README.md still mentions global escrow even though it has been replaced with pool-specific escrow, which can be confusing for future auditors and integrators:

• "The vault enforces controller/owner validation for all operations and integrates with the global escrow for asset custody"
• "coordinating with BalanceSheet for share issuance/burning and the global escrow for asset custody."
• Also still present in some diagrams

Informational.

Update the README and diagrams to reference pool-specific escrow changes made.

Fixed by bd06c52.

AsyncRequestManager.maxMint defines the shares named return parameter but fails to assign a value to it.

Informational.

Maintain consistency with the rest of the max* methods and assign the return value to shares:

@@ -554,7 +554,7 @@ contract AsyncRequestManager is Auth, IAsyncRequestManager, ITrustedContractUpda
         if (!_canTransfer(
                 vault_, address(balanceSheet.escrow(vault_.poolId())), user, uint256(investments[vault_][user].maxMint)
             )) return 0;
-        return uint256(investments[vault_][user].maxMint);
+        shares = uint256(investments[vault_][user].maxMint);
     }
 
     /// @inheritdoc IRedeemManager

Fixed by 2706805.

ShareClassManager.addShareClass now requires the salt parameter's first 8 bytes to match the provided poolId value, in order to prevent salt values previously used for a pool from being used for a different pool.

The method also ensures that a given salt is used only once, by storing used values in its ShareClassManager.salts mapping and checking a new salt against such set: if the salt has already been used, a AlreadyUsedSalt error is returned.

Both checks may be moved to the beginning of the method, in order to marginally reduce the gas consumed by the method in both unhappy paths.

Gas optimization.

Move the code related to the mentioned checks to the beginning of the method:

@@ -38,6 +38,10 @@ contract ShareClassManager is Auth, IShareClassManager {
         auth
         returns (ShareClassId scId_)
     {
+        PoolId prefixedPoolId = PoolId.wrap(uint64(bytes8(salt)));
+        require(poolId == prefixedPoolId, InvalidSalt());
+        require(!salts[salt], AlreadyUsedSalt());
+
         scId_ = previewNextShareClassId(poolId);
 
         uint32 index = ++shareClassCount[poolId];
@@ -45,9 +49,6 @@ contract ShareClassManager is Auth, IShareClassManager {
 
         ShareClassMetadata storage meta = _updateMetadata(poolId, scId_, name, symbol);
 
-        PoolId prefixedPoolId = PoolId.wrap(uint64(bytes8(salt)));
-        require(poolId == prefixedPoolId, InvalidSalt());
-        require(!salts[salt], AlreadyUsedSalt());
         salts[salt] = true;
         meta.salt = salt;

Fixed by 9eb2a2d.

The reserve() and unreserve() functions load the reservedBy mapping twice when they could load it once by declaring newReservedAmount earlier in the function.

Gas savings.

Declare newReservedAmount earlier and use it instead of incrementing.

function reserve(...) external auth {
+   uint128 newReservedAmount = reservedBy[scId][caller][reason][asset][tokenId] + value;
-   reservedBy[scId][caller][reason][asset][tokenId] += value;
+   reservedBy[scId][caller][reason][asset][tokenId] = newReservedAmount;
    holding_.reserved += value;

-   uint128 newReservedAmount = reservedBy[scId][caller][reason][asset][tokenId];
    emit IncreaseReserve(asset, tokenId, poolId, scId, caller, reason, value, newReservedAmount);
}

function unreserve(...) external auth {
+   uint128 newReservedAmount = reservedBy[scId][caller][reason][asset][tokenId] - value;
-   reservedBy[scId][caller][reason][asset][tokenId] -= value;
+   reservedBy[scId][caller][reason][asset][tokenId] = newReservedAmount;
    holding_.reserved -= value;

-   uint128 newReservedAmount = reservedBy[scId][caller][reason][asset][tokenId];
    emit DecreaseReserve(asset, tokenId, poolId, scId, caller, reason, value, newReservedAmount);
}

Fixed by 85efcfc.