Centrifuge - rokinot's results

Low

Factory.sol: No zero length check for wards

If the wards array is empty, the LP will permanently have no wards and will need to be reimplemented in the future in order to be pausable. Worst case scenario, the protocol isn't able to address this issue before the LP gets more traction. Consider adding a fix similar to the following:

    require(awards.length != 0, "revert string");

InvestmentManager.sol: Pools are not fully ERC4626 compliant

According to EIP-4626, functions with the purpose of calculating how many tokens someone will earn by withdrawing shares and how many shares someone earns by depositing X tokens should be rounded up, however the protocol is using the same logic as previewDeposit and previewRedeem, which is to round down.

In all functions, _calculateCurrencyAmount() is called, with the following lines of code:

uint256 currencyAmountInPriceDecimals = _toPriceDecimals(
            trancheTokenAmount, trancheTokenDecimals, liquidityPool
        ).mulDiv(price, 10 ** PRICE_DECIMALS, MathLib.Rounding.Down);

As a result, all calculations are rounded down. In order to fix this, create a conditional that allows you to choose whether the function rounds up or down.

Escrow.sol: Transactions from/to it will stop working if a ward modifies the approval

The approval function below can be found in the escrow contract:

    function approve(address token, address spender, uint256 value) external auth { 
        SafeTransferLib.safeApprove(token, spender, value); 
        emit Approve(token, spender, value);
    }

The issue here is everyt ime safeApprove() is called with a value below typeof(uint256).max, any further transactions that does not decrease its allowance to zero will revert. For example, everytime handleTransfer() is called in the pool manager, its allowance is increased, and the transfer takes place, which instantly decreases it. If its allowance is higher than zero, every call to this function will fail.

Root.sol: Scheduled rely cannot be executed on current time

In executeScheduledRely(), the following check is made in order to guarantee it's time of execution is right.

        require(schedule[target] < block.timestamp, "Root/target-not-ready");

The line above is the validation check of the function. Notice that if the scheduled time is exact, the transaction will revert. This could happen in L2s such as arbitrum where there are multiple blocks every second.

To fix this, consider rewriting as followed:

        require(schedule[target] <= block.timestamp, "Root/target-not-ready");

Non Critical

Factory.sol: Token address shouldn't be assumed to be equal in all evm chains

Certain EVM chains, like zkSync, can use a different formula for derivating the address of a deployed contract, meaning if the project decides to deploy on said chain, the comment here will be inaccurate.

For more information: zkSync docs

Moreover, msg.sender will be a different address if a transaction to deploy the tranche token is sent to a sequencer on an optimistic rollup, from the L1 as mentioned in my other issue.

For more information: Arbitrum docs

Auth.sol: Consider adding a check to prevent the last ward from calling deny()

If the contract deployer calls deny() on itself, the pausing functions are lost, as there are no more wards to execute their functions.

ERC20.sol: Consider changing the contract's version

Version 3 is the current version of the Dai stablecoin, which is the basis for the contract. This variable should be reconsidered as it is the first interaction in this contract.

Root.sol: Consider adding a check to see whether the rely address is a contract

The function is intended to assign values to contracts only. The protocol may want to consider adding a check to see if the byte code size is higher than zero to guarantee the address is in fact a contract.

Make sure to always compile with the same flags as this contest

Right now, in the foundry.toml file, we can see the project is using the following flags:

solc_version = "0.8.21"   # Override for the solc version (setting this ignores `auto_detect_solc`)
# evm_version = "paris" # to prevent usage of PUSH0, which is not supported on all chains

This shows us the protocol is aware of breaking changes coming from solidity v0.8.21, and the shangai update as well. As a consequence, in the future, these flags must continue to be used as well if the protocol does not plan to go through another audit.

Introduction

Centrifuge is a protocol which works with RWAs and attempts to utilize DeFi's mechanics to it's advantage. The project is a two-way messaging blockchain, with most of it's accountability happening in the deployed chain of choice. Axelar is used as the "layer zero" in here, in order to provide an architecture in which it can execute the exchange said messages.

Approach taken in evaluating the codebase and evaluation

By starting with a manual review of the code itself, it's clear the codebase is highly dependent on permissioned interactions. With this in mind, attacks which could be coming from the general public were mostly out of question. This also means that in order to find other vulnerabilities, a more systemic approach should be given, as the protocol is dependent on it's bedrock infrastructure.

Architecture recommendations

Axelar does not cover many edge case scenarios, and the protocol currently is entirely dependent on it outside EOA action providers. Due to the nature of the protocol, a complete work around may not be possible without refactoring large areas of the code, however in certain cases it should be doable, one example being approaches to deal with an offline sequencer as mentioned in another report.

Centralization risks

The protocol is highly centralized, mainly due to the nature of the services which are being provided. This is within expectation, as RWA cannot be brought on chain without heavy restrictions in order to comply with security laws. With that said, . An analysis on the centrifuge chain would be required in order to more deeply assert how centralized the chain is, as parachains from polkadot can differ in this regard from one another, however that is outside the scope of this audit.

Systemic risks

Besides the ones mentioned previously, or in the contest main page, there are a few risks which can be brought from users, rather than developers. Let's say for example an user decides to transfer their token shares to a different address, and claims it to be done by mistake. The sponsors should be able to redeem the user by creating more of these tokens. However a malicious entity could have sent that to an address they already own, doubling their tokens.

There is also a risk of compliancy with the law as well. Recent events have shown that DeFi protocols which attempt to comply with the US law have also a tendency to be persecuted by said laws, such as WOO. As an auditor however, I am not near qualified enough to recommend any sort of prevention or mitigation.

Time spent:

12 hours