Impossible Cloud Network Protocol & Link Smart Contracts

Description

Users can create bookings for resources on ScalerNodes. The BookingManager is responsible for updating the utilized capacity trackers for the region, cluster and hardware provider for the node that's being booked/re-booked. As a booking expires the updates to these trackers are not updated immediately, instead a call to expireCapacity() needs to be made which will perform the desired operations.

Currently a node can be removed by a call to removeScalerNode() on ICNRegistry which does not check whether the capacity for the node is still marked as utilized. After removing the node it will be impossible to revert the utilized capacity updates that were made in creating the last booking for the removed node.

As a result various computations around protocol rewards will be inaccurate since they'll operate with inaccurate utilized capacity numbers.

Recommendation

Adjust the implementation of removeScalerNode() with a check that ensures a node can only be removed if there is no booking that still needs to be expired.

Description

Users are guarded against unexpected price increases in the bookCapacity function by specifying a maxBookingPrice. But this guard is not present in the extendBooking function where similar similar issue can occur

Recommendation

Add a similar guard in extendBooking

Description

The modules supporting link staking and reward claiming assume that link token ids have a value lower than the max uint32 value.

Currently the ICN link contract supports minting tokens that do not satisfy this constraint, as a result governance might accidentally mint tokens that are incompatible with the modules.

Recommendation

Introduce a require check that ensures that _mint only succeeds for tokens that have valid token ids.

Description

The batchInitiateDelegationRewardsClaim function only checks that the sum of _unclaimedRewards has to be non-zero. This allows individual claims to have 0 as the reward amount due to rounding in which case the checkpoints are still incremented. This can cause users to lose dust amount of rewards continuously

Recommendation

Revert in case a single _claimAmount is 0

Description

The DelegationCreated event is supposed to emit the index of the locked delegation rather than the node delegation's index. But the implementation currently emits the index of the nodeDelegation

/// @notice Emitted when a delegation is created.    /// @param nodeId The ID of the node    /// @param delegator The address of the delegator    /// @param lockedDelegationIndex The index of the locked delegation    /// @param apyScalingFactor The APY scaling factor for the delegation    /// @param unlockTimestamp The timestamp when the collateral can be unlocked    /// @param amount The amount of collateral delegated    event DelegationCreated(        uint256 indexed nodeId,        address indexed delegator,        uint256 indexed lockedDelegationIndex,        uint256 apyScalingFactor,        uint256 unlockTimestamp,        uint256 amount    );

Recommendation

Emit the UserDelegation index

Description

The store32() function is used to efficiently store an array of integers in storage.

There is a notable difference between the input type and output type. Namely the storage array is for uint32 and the input array contains uint256. It functions correctly if all numbers in the input array are smaller than type(uint32).max. However, the functions' behavior becomes undefined if the input has larger integers.

Since the parameter types allow uint256 the function should function under all potential values of that type.

Recommendation

Introducing a require ensuring the input values are lower than the maximum uint32 value or changing the input type to an array of uint32 would ensure the function has no undefined or inaccurate behavior.

Description

The architecture of ICN is aimed at having a single upgradable proxy contract that has multiple modules responsible for parts of the protocol. As expected there are some dependencies between modules. For example, the BookingManager needs to know if a node is bookable and will depend on the ICNRegistry module for that.

Each module is accompanied by a storage contract which provides an interface to the storage domain for that module. Currently most cross-module operations will perform direct look-ups and mutations to the storage of other modules.

This creates a situation where the responsibilities and dependencies between modules can be more opaque. For example, the BookingManager is responsible for recording the utilized capacity of resources otherwise managed by the ICNRegistry. In bug #1 we see that this leads to a potential misaccounting of capacities since ICNRegistry does not account for utilized capacity.

Recommendation

It is worth exploring the introduction of internal functions to storage contracts that wrap external reads and writes to make cross-module dependencies explicit. This will not prevent bugs but can help with code understanding.

Description

According to the documentation rewards should be distributed as

where

• $x_{n_i}(t)$ is the total capacity of the node $n_i$ at the time $t$.

But the implementation uses targetCapacity in the denominator rather than ∑$x_{n_i}(t)$

Recommendation

Correct the documentation

Description

The ICN codebase has several aspects that are quite amenable to fuzz testing and (bounded) formal verification.

The following are some example areas:

• bitmap implementation used for efficient batch staking and initial rewards computation
• complex arithmetic operations
• library implementation for array copies

testability

To make introducing fuzz testing easier it can help to isolate (more) pure logic in libraries. This also aids in building a regular test suite and scaling the codebase as it grows in complexity.

For example, the logic that structures the link initial reward bitmap and the link staking bitmap implements is roughly equivalent and generalizable. Though the operations are relatively straightforward, it is nice to be able to test such low level operations directly. Implementing a bitmap library that efficiently supports single & batch operations can greatly simplify the link modules while simultaneously allowing for extensive unit tests and techniques such as fuzzing.

fuzzing invariants

The areas of interest noted above can be tested in a stateless fashion, this is generally easier both on the developer and the fuzzing tool.

The following are example invariants that you might consider introducing fuzz tests for:

1. given two differing id's in the bitmap domain (e.g. link ids):
   1. marking the first will only work if it is not marked.
   2. marking only the first will not set the second as marked.
2. given any list of integers the store32 function will populate a list in storage where for every index the original array and storage array will have the same value.

stateful fuzzing

There is also an opportunity for stateful fuzzing.

For example bug #1 might have been discovered by the following invariant:

The utilized capacity for a region/hp/cluster is equal to the sum of recorded utilized capacity of those nodes that belong to the region/hp/cluster.

Similar invariants might be formulated for other aspects of cross module accounting.

Note that stateful fuzzing requires a more complex setup, so we recommend focussing on the invariants above first.

Description

Swapping the final ScalerNode with the to-be-deleted node should only be necessary when the to be deleted node is not the last node in the array.