Description

The emit Transfer(receiver, address(0), amount) in transferFrom() simulates burning IOU tokens from the receiver.

However the I Owe You promise is settled from the sender perspective, so more logical to use the sender.

This can confuse the offchain logic.

Recommendation

Consider changing the code to:

- emit Transfer(receiver, address(0), amount);
+ emit Transfer(sender, address(0), amount);

Description

In _processRedeem(), receiver can be arbitrarily selected via redeem() and withdraw().

RestrictionManager and RestrictedRedemptions both have the following restrictions on from:

if (uint128(hookData.from).getBit(FREEZE_BIT) == true && !root.endorsed(from)) {
    // Source is frozen and not endorsed
    return false;
}

The caller of redeem() and withdraw() can choose a receiver that doesn't have the FREEZE_BIT set, so the _canTransfer() check can be circumvented.

Note: this assumes address(0) doesn't have the FREEZE_BIT set, but if that would be the case _processRedeem() would never work.

Additionally claimCancelDepositRequest() also transfers assets but has no checks and receiver can also be arbitrarily selected.

Recommendation

Consider adding a _canTransfer() check to both _processRedeem() where controller is used as the from.

Also add similar checks in claimCancelDepositRequest().

Note: this requires adding controller as a parameter to _processRedeem().

Note: this is similar to the check on controller in InvestmentManager::requestRedeem().

A possible solution as suggested by the project is adding the following:

   if (controller != receiver) {
        require(
            _canTransfer(vault, controller, receiver, convertToShares(vault, assetsDown)),
        }
   } 

Description

InflationMinter uses an import from forge-std, which make the source depend on the used toolset (e.g. foundry).

Recommendation

If you don't want to rely on the toolset, consider importing IERC20 from one of the following:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol
• https://github.com/centrifuge/liquidity-pools/blob/main/src/interfaces/IERC20.sol

Description

The interface for function burn() isn't used in InflationMinter so it could be removed.

Recommendation

Consider removing the interface for burn():

- function burn(address user, uint256 value) external;

Description

The constructor of InflationMinter doesn't have sanity checks. Its very unlikely that the contract will be deployed with incorrect parameters and if it is, it can be redeployed, so the risk is low.

Recommendation

If you do want to add sanity checks, the following can be checked:

• target != address(0)
• token != address(0)
• decimals() exists (isn't mandatory in ERC20)
• period != 0 (that would result in a revert)
• rate is far smaller than ONE (a high inflation rate could result in overflows)

Description

The interface for function burn() in contract IouCfg is different than the standards from OpenZeppelin.

Recommendation

Consider using the standards from OZ ERC20Burnable:

• function burn(uint256 value) external;
• function burnFrom(address account, uint256 value) external;

Note: burnForm() requires an allowance and burn() doesn't

Note: this assumes the contract for newCfg still has to be designed/isn't final yet.

Description

The contracts IouCfg and InflationMinter use different names for the same interface:

• IouCfg uses InflationMinter
• InflationMinter uses IERC20Mintable

Recommendation

Consider using the same names in both contracts.

Description

The constructor in contract IouCfg uses: Auth(auth). It is not immediately clear what the difference between Auth and auth is.

Recommendation

Consider using initialWard instead of auth:

- constructor(address auth, address escrow_, address newCfg_, address legacyCfg_) Auth(auth) {
+ constructor(address initialWard , address escrow_, address newCfg_, address legacyCfg_) Auth(initialWard) {

Description

The functions updateMember() in RestrictedRedemptions and RestrictionManager update the timestamp and the FREEZE_BIT. However they don't retreive and maintain the remaining 63 bits of the hookData, so the unused bits are erased.

If more bits would be used in the future it might be useful to keep them.

Recommendation

Consider keeping the unused bits from hookData.

Description

The following events start with receiver and then controller. However most events start with controller, followed by receiver.

emit CancelDepositClaim(receiver, controller, REQUEST_ID, msg.sender, assets);
emit CancelRedeemClaim(receiver, controller, REQUEST_ID, msg.sender, shares);

Recommendation

Consider using the same order in all events.

Description

Function ERC7540Vault::requestRedeem() calls checkTransferRestriction() and InvestmentManager::requestRedeem(). Function InvestmentManager::requestRedeem() calls _canTransfer(), which is similar to checkTransferRestriction().

So two similar checks are done in different places.

Recommendation

Consider putting the checks in the same location, which is easier to read and maintain.

Description

1. The following comment in InvestmentManager::_canTransfer() can be improved to make it more readable:

- Sender (from) and receiver (to) have both to pass
+ Sender (from) and receiver (to) have to both pass

Description

Comments in the withdraw() and redeem() functions state: DOES NOT support controller != msg.sender

However, this is not completely accurate. msg.sender can be different than the controller if msg.sender happens to be an operator of controller.

Recommendation

Consider updating the comment to make this clear.

Description

The functions Redeem() and Withdraw() call _processRedeem(), which now has a _canTransfer() check.

The related functions maxRedeem() and maxWithdraw() don't have such a check

However the functions maxMint() and maxDeposit() , which are similar to maxRedeem() and maxWithdraw(), do have a _canTransfer() check.

Recommendation

Consider adding a _canTransfer() check to maxRedeem() and maxWithdraw().

Description

A comment in IouCfg::transferFrom() can be improved to make it more readable:

- "Ensures that only for Centrifuge to Eth tokens are minted"
+ "Ensures that only for the direction "Centrifuge to Eth", tokens are minted"

Description

Function mint() of InflationMinter will be used regularly (daily) on mainnet. It can be optimized to same gas costs.

Recommendation

Consider using one or more of the following optimizations:

• replace uint64 with uint256;
• replace _rpow() with solady rpow , which will save ~73 gas and saves duplicating code;
• perform the calculation in periods, which will save ~1100 gas. This can be done in the following way:

    uint256 public immutable period = ...;
    uint256 public lastMintingInPeriods = block.timestamp/period;
    function mint() external {
        uint256 elapsedPeriods = block.timestamp/period;
        uint256 cached = lastMintingInPeriods;
        require(elapsedPeriods > cached, "InflationMinter/already-inflated");   
        uint256 mintingsElapsed = elapsedPeriods - cached;    
        lastMintingInPeriods = elapsedPeriods;
        ... // use lastMintingInPeriods 
    }

Description

Function decimals() queries the decimals() from legacyCfg every time. This could be optimized.

Recommendation

Consider retrieving the value for IERC20Metadata(legacyCfg).decimals() in the constructor and store the value in an immutable variable.

Description

Several functions in ERC7540Vault as public while they are not called from the contract itself:

function requestDeposit(uint256 assets, address controller, address owner) public returns (uint256) {
function pendingDepositRequest(uint256, address controller) public view returns (uint256 pendingAssets) {
function requestRedeem(uint256 shares, address controller, address owner) public returns (uint256) {
function pendingRedeemRequest(uint256, address controller) public view returns (uint256 pendingShares) {
function pendingCancelDepositRequest(uint256, address controller) public view returns (bool isPending) {
function claimableCancelDepositRequest(uint256, address controller) public view returns (uint256 claimableAssets) {
function pendingCancelRedeemRequest(uint256, address controller) public view returns (bool isPending) {
function claimableCancelRedeemRequest(uint256, address controller) public view returns (uint256 claimableShares) {
function setOperator(address operator, bool approved) public virtual returns (bool success) {
function setEndorsedOperator(address owner, bool approved) public virtual {
function convertToShares(uint256 assets) public view returns (uint256 shares) {
function maxDeposit(address controller) public view returns (uint256 maxAssets) {
function maxMint(address controller) public view returns (uint256 maxShares) {
function mint(uint256 shares, address receiver) public returns (uint256 assets) {
function maxWithdraw(address controller) public view returns (uint256 maxAssets) {
function withdraw(uint256 assets, address receiver, address controller) public returns (uint256 shares) {
function onRedeemRequest(address controller, address owner, uint256 shares) public auth {
function onDepositClaimable(address controller, uint256 assets, uint256 shares) public auth {
function onRedeemClaimable(address controller, uint256 assets, uint256 shares) public auth {
function onCancelDepositClaimable(address controller, uint256 assets) public auth {
function onCancelRedeemClaimable(address controller, uint256 shares) public auth {

Note: the following functions are called from the contract, so should stay public:

function DOMAIN_SEPARATOR() public view returns (bytes32) {
function convertToAssets(uint256 shares) public view returns (uint256 assets) {
function deposit(uint256 assets, address receiver, address controller) public returns (uint256 shares) {
function mint(uint256 shares, address receiver, address controller) public returns (uint256 assets) {
function maxRedeem(address controller) public view returns (uint256 maxShares) {

Recommendation

Consider making these functions external to save some gas.