Olas - 0xTheC0der's results

Judge has assessed an item in Issue #146 as 3 risk. The relevant finding follows:

L-6: OLAS minting via treasury is not guaranteed The OLAS.mint(…) method does not revert if the requested amount cannot be limited due to the inflation limit: function mint(address account, uint256 amount) external { // Access control if (msg.sender != minter) { revert ManagerOnly(msg.sender, minter); }

    // Check the inflation schedule and mint
    if (inflationControl(amount)) {
        _mint(account, amount);
    }
}

The treasury itself correctly accounts for the OLAS inflation limit and thereby guarantees the minting of the the requested OLAS amount here and here. However, the minter of OLAS can be changed via changeMinter(address newMinter) by the owner. Then, the new minter can mint OLAS up to the inflation limit. Next, the minter is changed back to the treasury contract. Now, the minting via treasury (see references in step 2) will fail to mint enough OLAS without any error.

Summary

• L-1: Front-running of setFxChildTunnel/setFxRootTunnel after deployment
• L-2: Inconsistent OLAS supply cap
• L-3: Treasury conservation law can be broken via selfdestruct
• L-4: Donator blacklisting is ineffective and can be circumvented
• L-5: Users can be DoS'ed on Depository.deposit(...) by front-runner
• L-6: OLAS minting via treasury is not guaranteed
• NC-1: Treasury.drainServiceSlashedFunds() can leave dust behind in ServiceRegistry
• NC-2: Add implementation of GuardCM.checkAfterExecution(...)

L-1: Front-running of setFxChildTunnel/setFxRootTunnel after deployment

The FxERC20ChildTunnel.setFxRootTunnel(...) and FxERC20RootTunnel.setFxChildTunnel(...) methods need to be called immediately after deployment of these 2 contracts in order to finish their initialization.

However, those methods' access is not restricted:

function setFxRootTunnel(address _fxRootTunnel) external virtual {
    require(fxRootTunnel == address(0x0), "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET");
    fxRootTunnel = _fxRootTunnel;
}

function setFxChildTunnel(address _fxChildTunnel) public virtual {
    require(fxChildTunnel == address(0x0), "FxBaseRootTunnel: CHILD_TUNNEL_ALREADY_SET");
    fxChildTunnel = _fxChildTunnel;
}

As a consequence, the initialization can be front-run by an attacker after deployment to redirect the tunnels.
However, this attack can only happen after deployment before the protocol is in use and needs to go unnoticed for a while to cause any damage. Otherwise, redeployment is straight-forward in case this happens.

Why is it implemented like this?

Because those 2 tunnel contracts are dependent on each other's address. One is on Ethereum, the other on Polygon.

How to solve this?

Deploy those contracts via create2 with a salt in order to ensure deterministic contract addresses before deployment.
Therefore, the pre-computed root/child tunnel address can already be provided via the constructor which avoids the need for a subsequent initialization call that can be front-run.

L-2: Inconsistent OLAS supply cap

The TokenomicsConstants.getSupplyCapForYear(...) method has pre-defined progressive supply cap levels for the first 10 years and 2% inflation afterwards, while the OLAS.inflationRemainder() method, which actually enforces the supply cap on minting, has the same fixed supply cap for the first 10 years and 2% inflation afterwards.
Therefore there is a discrepancy between the amount of OLAS which is expected to be minted vs. the amount which actually can be minted within the first 10 years.

The expected supply cap levels for the first 10 years from TokenomicsConstants.sol:

uint96[10] memory supplyCaps = [
    529_659_000_00e16,
    569_913_084_00e16,
    641_152_219_50e16,
    708_500_141_72e16,
    771_039_876_00e16,
    828_233_282_97e16,
    879_860_040_11e16,
    925_948_139_65e16,
    966_706_331_40e16,
    1_000_000_000e18
];

The actual supply cap for the first 10 years from OLAS.sol:

uint256 public constant tenYearSupplyCap = 1_000_000_000e18;

L-3: Treasury conservation law can be broken via selfdestruct

According to Treasury.sol:

/// invariant {:msg "broken conservation law"} address(this).balance == ETHFromServices + ETHOwned;

Although the contract's constructor and methods perfectly respect this invariant, one can simply force-fund the contract with ETH using the selfdestruct EVM opcode which consequently breaks the given invariant.

Recommendation: Add a sync() method which adds any surplus balance to the ETHOwned state variable while respecting the type(uint96).max limit, see also receive().

L-4: Donator blacklisting is ineffective and can be circumvented

A blacklisted donator can simply circumvent the blacklisting by using another account for donations. This is already proven within the test case "Claim incentives for unit owners when donator and service owner are different accounts" which shows that the donator can be an unrelated account.

The only point where the actual address of the donator is important is in Tokenomics._trackServiceDonations(...):

bool topUpEligible;
if (incentiveFlags[2] || incentiveFlags[3]) {
    address serviceOwner = IToken(serviceRegistry).ownerOf(serviceIds[i]);
    topUpEligible = (IVotingEscrow(ve).getVotes(serviceOwner) >= veOLASThreshold  ||
        IVotingEscrow(ve).getVotes(donator) >= veOLASThreshold) ? true : false;
}

Here, the top-up eligibility is determined based on the votes of the service owner or donator.
Therefore, the blacklisting is only effective if the service owner does not have enough votes and the alternative donator (to circumvent blacklist) does not have enough votes either.

L-5: Users can be DoS'ed on Depository.deposit(...) by front-runner

Anyone can front-run a user's Depository.deposit(uint256 productId, uint256 tokenAmount) transaction.
However, this is especially a problem whem the user desires a full deposit where tokenAmount is chosen such that the whole product.supply is used. In this case, the attacker can front-run the transaction with a miniscule tokenAmount in order to cause DoS for the user via a ProductSupplyLow error.

L-6: OLAS minting via treasury is not guaranteed

1. The OLAS.mint(...) method does not revert if the requested amount cannot be limited due to the inflation limit:

    function mint(address account, uint256 amount) external {
        // Access control
        if (msg.sender != minter) {
            revert ManagerOnly(msg.sender, minter);
        }

        // Check the inflation schedule and mint
        if (inflationControl(amount)) {
            _mint(account, amount);
        }
    }

2. The treasury itself correctly accounts for the OLAS inflation limit and thereby guarantees the minting of the the requested OLAS amount here and here.
3. However, the minter of OLAS can be changed via changeMinter(address newMinter) by the owner.
4. Then, the new minter can mint OLAS up to the inflation limit.
5. Next, the minter is changed back to the treasury contract.
6. Now, the minting via treasury (see references in step 2) will fail to mint enough OLAS without any error.

NC-1: Treasury.drainServiceSlashedFunds() can leave dust behind in ServiceRegistry

Since the drainServiceSlashedFunds() method enforces the minAcceptedETH limit before draining funds from the ServiceRegistry. There can be occasions where small amounts (< minAcceptedETH) are left behind in the ServiceRegistry without any possibility to withdraw them.

Recommendation: Do not enforce minAcceptedETH when draining slashed funds.

NC-2: Add implementation of GuardCM.checkAfterExecution(...)

The implementation of the GuardCM.checkAfterExecution(...) method, which is the counterpart to GuardCM.checkTransaction(...), is empty.
There might be settings within the protocol that should not be changed via a multisig call. Thus, it is recommended to check for such invariants after the call within that method and revert if necessary.