StakingImplementation Manual Review Findings

SIN-01M: Potential Denial of Service

Type	Severity	Location
Language Specific		StakingImplementation.sol:L156

Description:

The claim operation of the staking implementation attempts to claim all submitted unstake operations without a limit specifier thus potentially leading to an inexecutable claim operation.

Impact:

The claim function may become un-invoke-able thereby permanently locking unstaked funds in the system.

Example:

Copy
146/**
147 * @notice Claim all unstakes that are older than cooldown
148 */
149function claim() external override nonReentrant {
150    Holder storage _holder = holders[msg.sender];
151
152    uint256 _index = _holder.nextUnstakeId;
153    uint256 _amount;
154
155    while (
156        _index < _holder.unstakes.length &&
157        _holder.unstakes[_index].cooldownBlock < block.number
158    ) {
159        _amount += _holder.unstakes[_index].amount;
160        _index++;
161    }
162
163    require(_amount > 0, "Stake::claim: No funds to claim");
164
165    _holder.nextUnstakeId = _index;
166
167    SPACT.burn(msg.sender, uint96(_amount));
168    PACT.safeTransfer(msg.sender, _amount);
169
170    emit Claimed(msg.sender, _amount);
171}

Recommendation:

We advise an optional argument to be specified that states how many claims should be attempted before the loop operation ends early to ensure that unstakes will always be claimed regardless of the block gas limit.

Alleviation:

A new claim function called claimPartial has been introduced to claim unstake entries up to a particular index thus avoiding a potentially inexecutable claim operation and alleviating this exhibit.

SIN-02M: Unsafe Casting Operations

Type	Severity	Location
Mathematical Operations		StakingImplementation.sol:L107, L167

Description:

The linked casting operations to uint96 are performed unsafely.

Impact:

An overflow in the staking operation would cause an improper SPACT amount minted whereas multiple stake operations could compound to a single claim with zero PACT minted due to an overflow cast.

Example:

Copy
97/**
98 * @notice Stakes new founds for the holder
99 *
100 * @param _holderAddress Address of the holder
101 * @param _amount Amount of cUSD tokens to stake
102 */
103function stake(address _holderAddress, uint256 _amount) external override nonReentrant {
104    require(_amount > 0, "Stake::stake: Amount can't be 0");
105
106    PACT.safeTransferFrom(msg.sender, address(this), _amount);
107    SPACT.mint(_holderAddress, uint96(_amount));
108
109    //.add method checks if the stakeholdersList already contains this address
110    stakeholdersList.add(_holderAddress);
111
112    holders[_holderAddress].amount += _amount;
113    currentTotalAmount += _amount;
114
115    donationMiner.setStakingAmounts(
116        _holderAddress,
117        holders[_holderAddress].amount,
118        currentTotalAmount
119    );
120
121    emit Staked(_holderAddress, _amount);
122}

Recommendation:

We advise a bound check to be performed before each casting operation to ensure that the value being cast fits within the uint96 data type (i.e. is less than type(uint96).max). We would like to note that Solidity's built-in safe arithmetics do not cover casting operations.

Alleviation:

The stake implementation has been updated to enforce a require check guaranteeing the staking amount fits into the uint96 data type thus fully alleviating this exhibit.