GLIF InfinityPoolV2 & AgentPoliceV2 Migration Security Review Report

The harvest function of the LiquidityMineSP can be used by an Agent's owner to withdraw any earned rewards of the Agent. It takes the Agent's address as agent parameter. However, this parameter is never validated and it is trusted to be a real Agent.

It makes external calls to this agent address for the id, owner and defaulted. But since this is an arbitrary address, these values can be spoofed by the caller.

Since msg.sender is only checked against the fetched owner, it can be bypassed and consequently the fetched id is then trusted. The corresponding Agent's rewards are used and send to the attacker.

Using this, the attacker can repeat on every agent and drain all of their rewards at once.

The harvest needs to validate that the provided agent is indeed a valid Agent using the AgentFactory:isAgent function.

The function InfinityPoolV2._redeem() allows stakers to redeem their shares for assets in two steps:

• It calculates the amount of assets (FIL) to be transferred to the staker by calling assets = previewRedeem(shares)
• It processes the exit by burning shares iFIL and transferring assets FIL to the staker through the _processExit() function. The issue occurs when the calculated assets in line 866 of the previewRedeem() function exceeds the contract's liquid assets (getLiquidAssets()). In this situation, previewRedeem(shares) will return 0 assets due to line 868, resulting in the transfer of 0 assets to the staker while still burning shares iFIL in the _processExit() function.

Consider reverting the _processExit() function if the assetsToReceive == 0.

The function InfinityPoolV2::previewRedeem() attempts to calculate the asset amount received from redeeming a specific share amount. However, it uses rounding up, which will incorrectly return more assets. The previewRedeem function should use rounding down, whereas previewWithdraw uses rounding up.

For example:

• The total shares are 10 and the total assets are 12
• If a user uses withdraw function to withdraw 2 assets, they will need 2 shares.
• If they use redeem function, they can redeem only 1 share to obtain 2 assets. Similarly, previewMint function triggers convertToAssets and calculates the assets needed to mint incorrectly by rounding down. It should use rounding up instead.

We recommend using rounding down for previewRedeem function and rounding up for previewMint function.

The UpgradeToV2.upgrade() function, responsible for the migration, is prone to DoS because of the internal call to mustBeEqual().

The call contains several checks, one of them is the following:

if (pool1State.totalAssets != pool2State.totalAssets) {
    revert TotalAssetsMismatch(pool1State.totalAssets, pool2State.totalAssets);
}

The calculations of totalAsset are based on the balance of a pool:

function totalAssets() public view override returns (uint256) {
    // pseudo accounting update to make sure our values are correct
    (uint256 newRentalFeesAccrued, uint256 newTFeesAccrued) = _computeNewFeesAccrued();
    // using accrual basis accounting, the assets of the pool are:
    // assets currently held by the pool
    // total borrowed from agents
    // total owed rental fees to LPs
    // subtract owed treasury fees
    return asset.balanceOf(address(this)) + totalBorrowed
        + _lpRewards.accrue(newRentalFeesAccrued).owed()
        - _treasuryRewards.accrue(newTFeesAccrued).owed();
}

During the migration, the balance of the old pool will be sent to the new one, and the check above ensures it stays the same.

Therefore, a malicious actor can send any number of asset directly to the new pool contract, influencing the accounting and disrupting the check, because old pool balance != old pool balance + 1 wei, for example.

We recommend removing the check.

While upgrading an agent, the upgradeAgent function doesn't check if the agent is defaulted, so a defaulted agent can still be upgraded to a new agent. However, the defaulted variable of the new agent won't be set to true, allowing it to still harvest rewards from the LiquidityMineSP contract. Additionally, the owner of a defaulted agent can use this action to prevent miners liquidation from the prepareMinerForLiquidation() function.

A defaulted agent shouldn't be allowed to upgrade, so a validation should be added to the upgradeAgent function.

When the protocol is updating the interest rate, it will pause operations. Due to transaction gas limits, this rate update process will be divided into multiple transactions. In each transaction, the epochPaid for accounts will be recalculated to reflect the new interest rate. However, the STORAGE _rentalFeesOwedPerEpoch is updated to the new rate only in the final transaction of the rate update process.

This method leads to incorrect DTL calculations in the AgentPoliceV2.setAgentDefaultDTL() function. The DTL is computed based on the current state of the account and the current getRate(), which equals _rentalFeesOwedPerEpoch. This causes a mismatch during the rate update process. Accounts that have already updated their epochPaid will use the new epochPaid along with the old interest rate to calculate the interestOwed, resulting in an incorrect amount. Consequently resulting in an unexpected liquidation.

Consider adding the whenNotPaused modifier to the function writeOff().

The rate update process involves two functions: startRateUpdate() and continueRateUpdate(). The startRateUpdate() function is called once, while continueRateUpdate() can be called multiple times. This two-step process is necessary because updating a large number of agents in a single transaction could exceed the gas limit.

However, this approach introduces a risk when startRateUpdate() and continueRateUpdate() are called in different blocks. The problem arises in the _updateAgentAccounts() function, where the interestOwed for an account is calculated from epochPaid to block.number using the old interest rate (line 1105). As a result, if continueRateUpdate() is triggered in a different block from startRateUpdate(), the old interest rate is applied to more blocks for accounts updated by startRateUpdate() than for those updated by continueRateUpdate(). This creates an unfair situation.

Add a new variable to the RateUpdate struct to store the block.timestamp when startRateUpdate() is triggered. This variable can then be used in continueRateUpdate() to calculate the interestOwed by the accounts.

struct RateUpdate {
    uint256 totalAccountsAtUpdate;
    uint256 totalAccountsClosed;
    uint256 newRate;
    bool inProcess;
+   uint rateUpdateStartBlock;
}

The following external functions will emit two Withdraw events:

• redeem(uint256 shares, address receiver, address owner)
• redeem(uint256 shares, address receiver, address owner, uint256) Because the internal _redeem() and the underlying _processExit() both contain it.

The same issue applies to the internal _withdraw(), but it's unused.

We recommend removing the event from the internal function.

The internal _computeAccRewards() function in LiquidityMineLP, responsible for updating the accrued rewards, uses lockToken.balanceOf(address(this)) to get the total amount of staked tokens.

In case some amount of lockToken was sent to the contract directly without using the deposit() function, the accounting will still accrue rewards to those tokens, decreasing the rewards for normal depositors. Since the rewards without a position cannot be withdrawn, they will be burned on shutdown.

We recommend using an internal storage variable to track the total available funds and adding a function to rescue the additional tokens.

The isValidReceiver modifier does not normalize the receiver argument, so it's possible to pass a Filecoin ID instead of an address to bypass the receiver == address(this) check.

We recommend normalizing the argument before the checks.

The setTreasuryFeeRate() function in InfinityPoolV2 is the setter for the treasuryFeeRate variable.

Under normal circumstances, it'll perform updateAccounting() before to accrue all the fees using the old rate.

But if called during a pause, it'll skip the update and set the variable directly, postponing the calculations to after the pause. When the pause is over, the next call to updateAccounting() will unfairly accrue all the fees with the new rate.

We recommend adding the whenNotPaused modifier to the function.

During the liquidation of an agent, the AgentPoliceV2::setAgentDefaultDTL() function checks the DTL ratio of the agent and sets the storage variable defaulted of the agent to true if the DTL is lower than the threshold.

After that, the AgentPoliceV2::distributeLiquidatedFunds() function is intended to be called, which triggers the InfinityPoolV2::writeOff() function to cover the liquidated debt. This function will set the account.defaulted state of the agent's account to true, making the account unable to repay.

function writeOff(uint256 agentID, uint256 recoveredFunds) external {
    // only the agent police can call this function
    _onlyAgentPolice();
    updateAccounting();
    Account memory account = _getAccount(agentID);
    if (account.defaulted) revert AlreadyDefaulted();
    // set the account to defaulted
    account.defaulted = true;
}

However, before calling writeOff, there is no validation to check that the agent was liquidated (the defaulted state of the agent needs to be true). This leads to risks of centralization or mistakes from the owner of AgentPoliceV2, as distributeLiquidatedFunds can be called and cause lost funds for a healthy agent.

A validation should be added to check the default state of the agent during distributeLiquidatedFunds.

The faultySectorStartEpoch storage variable will always equal 0 because AgentPoliceV2 does not use its setter, the setFaulty() function.

The variable may then be unnecessarily reset in setRecovered().

We recommend removing the variable and upgrading the Agent, if it's not used by the oracle.

There are multiple mentions of the InfinityPoolV2 being an ERC4626 vault, but it does not implement any ERC20 functions.

As you can see in the EIP proposal:

All EIP-4626 tokenized Vaults MUST implement EIP-20 to represent shares.

This means there should be functions available such as transfer(), balanceOf(), name(), etc., in order to conform with the standard.

We recommend the functions' addition or removal of the comments.

You can use the implementation by OpenZeppelin as a reference: ERC 20 - OpenZeppelin Docs

In the LiquidityMineLP contract, the withdrawAndHarvest function is used to withdraw LP tokens and harvest rewards for users in one transaction. However, it uses the same variable to represent both the LP token amount to withdraw and the reward token amount to harvest, even though they are amounts of different tokens. This leads to users being unable to choose specific amounts for withdrawal and harvesting by this function.

We recommend adding another parameter for the harvest amount.

Regarding the Agent contract, the process for upgrading the agent is as follows: the owner calls AgentFactory::upgradeAgent(), which then calls Agent::decommissionAgent() and sets up newAgent. Afterward, the miner of the old agent should be migrated to the new agent using the migrateMiner function in the old Agent contract.

However, this function requires msg.sender to be newAgent, and an Agent contract does not have the ability to call this function.

if (newAgent != msg.sender) revert Unauthorized();

To enable migration, the workflow for miner migration should resemble the upgrading process, where the owner can call the AgentFactory, and the AgentFactory will in turn call Agent::migrateMiner() to change the miner's new agent.

The new agent should have the ability to call the miner, or you can move the responsibility for miner migration to the AgentFactory.

The internal _withdraw() function is not used anywhere in the system. The external functions duplicate the internal's functionality.

We recommend modifying the _withdraw() function to accept bool shouldConvert and replacing the duplicate code with this function in:

• withdraw(uint256 assets, address receiver, address owner)
• withdraw(uint256 assets, address receiver, address owner, uint256)
• withdrawF(uint256 assets, address receiver, address owner)
• withdrawF(uint256 assets, address receiver, address owner, uint256)

The function InfinityPoolV2.deposit() calculates the number of shares a user will receive using the function previewDeposit() -> convertToShares(), which utilizes the round-down formula:

shares = assets.mulDivDown(supply, totalAssets())

where:

• assets: the amount of asset tokens the user will use to mint shares
• totalAssets(): the balance of asset tokens in the liquidity pool contract
• supply: the current total supply of liquidStakingToken The issue arises when the totalAssets() function is calculated directly using a call to asset.balanceOf(address(this)) in line 234. This flaw can be exploited by the first depositor, as outlined in the following attack strategy.

We recommend that the contract implements a mechanism similar to UniswapV2, which transfers the first 1000 LP tokens to address(0).