Centrifuge v3.1 Upgrade

The calculation of the NAV is given by the netAssetValue() function.

213:         return equity + gain - loss - liability;

If the liabilities are eventually greater than the adjusted equity, the calculation would overflow since these are unsigned integers. This can cause a revert in the path that submits snapshots to the Hub, which are hooked to the NAVManager.

Medium. The issue can block updates to the Hub for as long as the calculation overflows.

Consider clamping the calculation to zero to avoid the revert.

Note that this could also skew the final NAV after being aggregated in the SimplePriceManager contract.

Fixed by PR#708.

The approveRedeems() function doesn't need to be payable since it doesn't dispatch a message.

Low.

Remove the payable modifier.

Fixed in PR#721.

The refactored maxRedeemClaims() function in BatchRequestManager uses the revoke field from the stored EpochId struct, but it should take the redeem value instead.

Low.

Change the revoke field for the redeem field.

Fixed by PR#720 which actually changes maxDepositClaims to use epochId[..].issue instead of *.deposit because otherwise maxDepositClaims returns 1 claimable epoch after approveDeposits while notifyDeposit fails with IssuanceRequired as long as the issuance is missing (i.e. epochId.issue < epochId.deposit). So before this fix, it incorrectly signals a possible deposit claim.

PR#675 introduces changes for SyncManager.maxMint() and SyncManager.maxDeposit() functions to take into account potential transfer restrictions imposed by a vault's share token's hooks.

The current SyncManager.maxDeposit() implementation passes the result of SyncManager._maxDeposit, which is an amount denominated in a vault's asset, as a parameter in a call to SyncManager._canTransfer() which expects an amount of vault shares.

Low. While current ITransferHook implementations do not impose transfer restrictions based on the amount of share tokens being transferred, passing an asset amount instead of a share amount may hinder correct functionality in future implementations.

Calculate the amount of corresponding shares using convertToShares(vault_, amount) as is done within SyncManager.maxMint().

Fixed by PR#723.

The implementation of closeGainLoss() ignores the isPositive return value when fetching the current state of the gain and loss accounts.

176:         (, uint128 gainValue) = accounting.accountValue(poolId, gainAccount_);
177:         (, uint128 lossValue) = accounting.accountValue(poolId, lossAccount_);

The function then proceeds to adjust the equity account based on these values. If there are gains, it debits the gain account and credits the equity. If there are losses, it credits the loss account (debit normal) and debits the equity.

Low. The accounting would be incorrect if the returned values are not positive.

Given the implementation assumptions, consider checking that the returned values are indeed positive.

Fixed in PR#728.

BaseTransferHook.isDepositRequestOrIssuance() may be used within a ITransferHook implementation to capture and execute arbitrary logic when vault share tokens are minted to an address different than BaseTransferHook.depositTarget.

During a cross chain transfer's delivery on the target chain, share tokens are first minted into the Spoke contract and then transferred to the recipient: link.

Given that the Spoke contract is assigned to BaseTransferHook.crosschainSource, which is know to be different BaseTransferHook.depositTarget, the mentioned predicate will return true during the share token mint.

Informational.

Modify the mentioned function to not return true when to == crosschainSource:

 @@ -109,23 +109,29 @@ abstract contract BaseTransferHook is Auth, IMemberlist, IFreezable, ITransferHo
     ) public view virtual returns (bool);
 
     function isDepositRequestOrIssuance(address from, address to) public view returns (bool) {
-        return from == address(0) && to != depositTarget;
+        return from == address(0) && to != depositTarget && to != crosschainSource;
     }

Fixed in PR#725.

The implementation of trustedCall() in BaseTransferHook doesn't switch on the kind field of the UpdateContractUpdateAddress struct, treating all messages as updates to the manager.

Informational.

Check if kind equals some constant and revert if not.

UpdateContractMessageLib.UpdateContractUpdateAddress memory m =
    UpdateContractMessageLib.deserializeUpdateContractUpdateAddress(payload);
    
if (m.kind == "manager") {
    address token = address(spoke.shareToken(poolId, scId));
    require(token != address(0), ShareTokenDoesNotExist());

    manager[token][m.what.toAddress()] = m.isEnabled;
} else {
    revert UnknownUpdateContractKind();
}

Fixed in PR#715.

OracleValuation.sol provides an implementation for trusted price oracles to update asset prices.

The comments at OracleValuation.sol#L20-L22 indicate that, to utilize the highlighted contract developers must use hub.updateFeeder(). While Hub.sol doesn't implement such method, the correct way to integrate such contract is by using Holdings.updateValuation.

Informational.

Correct the documentation as shown.

Fixed in PR#725.

HubRegistry.updateCurrency may be used by an authorized address to update the HubRegistry.currency mapping.

The method fails to ensure that the new currency_ being linked to a given poolId_ has been previously registered via HubRegistry.registerAsset.

Informational.

Ensure currency_ has been registered:

@@ -87,6 +89,7 @@ contract HubRegistry is Auth, IHubRegistry {
     function updateCurrency(PoolId poolId_, AssetId currency_) external auth {
         require(exists(poolId_), NonExistingPool(poolId_));
         require(!currency_.isNull(), EmptyCurrency());
+        require(isRegistered(currency_));
 
         currency[poolId_] = currency_;

Fixed as recommended in PR#724.

The expression to check if the delay has elapsed can be simplified to the third sub-expression.

74:         require(
75:             sc.lastSync == 0 || sc.minDelay == 0 || block.timestamp >= sc.lastSync + sc.minDelay, MinDelayNotElapsed()
76:         );

For the duplicate check in the assetIds array, the scId argument can be removed, as this is constant for all elements. The validation can be simplified by ensuring asset IDs are in ascending order instead of using temporal storage. The check can also be removed because calling submitQueuedAssets() would blank deposits and withdrawals, causing the same asset ID to be skipped if repeated.

80:         for (uint256 i = 0; i < assetIds.length; i++) {
81:             bytes32 key = keccak256(abi.encode(scId.raw(), assetIds[i].raw()));
82:             if (TransientStorageLib.tloadBool(key)) continue; // Skip duplicate
83:             TransientStorageLib.tstore(key, true);
84: 
85:             // Check if valid
86:             (uint128 deposits, uint128 withdrawals) = balanceSheet.queuedAssets(poolId, scId, assetIds[i]);
87:             if (deposits > 0 || withdrawals > 0) {
88:                 balanceSheet.submitQueuedAssets(poolId, scId, assetIds[i], sc.extraGasLimit, address(0));
89:                 validCount++;
90:             }
91:         }

Additionally, the call to queuedShares() can be moved below the assets loop to fetch the updated value of queuedAssetCounter instead of tracking the validCount and then comparing it against the cached value of queuedAssetCounter.

Gas savings.

Consider implementing the recommended suggestions.

Fixed in PR#726.