CapitalAccounting Manual Review Findings

CAG-01M: Unsafe Delta Operations

Type	Severity	Location
Mathematical Operations		CapitalAccounting.sol:L36, L40, L44

Description:

The linked delta operations performed that are meant to either increase or decrease the previous value that the latest snapshot has are incorrect as they unsafely cast either the changeAmount variable or the result of its operation to a uint256 without validating that the resulting value is positive.

Impact:

In the current implementation, it is possible to update the capital snapshot of an account to a negative value as the subtraction would not underflow and would instead result in a negative value that is casted to a uint256, an operation that is not performed safely even in post-0.8.X Solidity versions.

Example:

Copy
34function _updateSnapshot(Snapshot[] storage snapshots, int256 changeAmount) private {
35    if (snapshots.length == 0) {
36        Snapshot memory snapshot = Snapshot(block.number, uint256(changeAmount));
37        snapshots.push(snapshot);
38    } else if (snapshots[snapshots.length - 1].blockNum < block.number) {
39        uint256 prevValue = snapshots[snapshots.length - 1].value;
40        Snapshot memory snapshot = Snapshot(block.number, uint256(int256(prevValue) + changeAmount));
41        snapshots.push(snapshot);
42    } else {
43        Snapshot storage snapshot = snapshots[snapshots.length - 1];
44        snapshot.value = uint256(int256(snapshot.value) + changeAmount);
45    }
46}

Recommendation:

We advise a safe casting operation to be performed in all three linked instances as otherwise the snapshot system is broken entirely.

Alleviation:

The CloudFunding team has stated that within the codebase the function is invoked with safe parameters and as such no underflow checks should be performed. As the contract represents functionality that arbitrary contracts can inherit from, including other projects, it is highly advisable to enforce proper bound checks for the operations and instead remove them from the upper calls. In any case, we consider this exhibit nullified as the code satisfies the use case of CloudFunding, does not pose an active threat to the system, and external projects cannot legally utilize the code without permission from CloudFunding in which case the dangers would be relayed.