The StakingToken is incompatible with fee-on-transfer tokens

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/e2a8bc31d2769bfb578a06cc64919ad369a82c08/registries/contracts/staking/StakingToken.sol#L117-L125

Vulnerability details

Impact

The [registries/contracts/staking/]StakingToken.sol (https://github.com/code-423n4/2024-05-olas/blob/main/registries/contracts/staking/StakingToken.sol#L117-L125) contract is incompatible with fee-on-transfer ERC20 tokens. The StakingToken contract assumes that the amount transferred during the deposit is received by address(this) in-full.

However, if the stakingToken is a fee-on-transfer ERC20 token, a percentage of the amount during the deposit will be consumed by the token contract as a charged fee.

Currently, the rewardToken is the Olas token, but, as per my clarification with @kupermind on Discord: "but obviously can be extended to any token".

Note that StakingToken is an extension for the StakingBase contract.

Due to the fact that the rewards accumulated for eligible stakers are calculated using the following formula, this problem with fee-on-transfer tokens being unsupported by the StakingToken extension contract (in case the rewardToken in StakingBase is one of them), will give incorrect results.

The precise damage might differ: for instance, for large StakingBase pools, keeping track of an improper totalRewards value can lead to serious calculation problems, whilst for smaller pools the calculation problem will not be that significant.

/** 623: **/         updatedReward = (eligibleServiceRewards[i] * lastAvailableRewards) / totalRewards;
/** 624: **/         // Add to the total updated reward
/** 625: **/         updatedTotalRewards += updatedReward;
/** 626: **/         // Add reward to the overall service reward
/** 627: **/         curServiceId = eligibleServiceIds[i];
/** 628: **/         finalEligibleServiceIds[i] = eligibleServiceIds[i];
/** 629: **/         finalEligibleServiceRewards[i] = updatedReward;
/** 630: **/         mapServiceInfo[curServiceId].reward += updatedReward;

Details

As a result of an improper calculation, the stakers will receive less rewards than they expect.

Taking into account the case when the rewardToken is a fee-on-transfer token, the user will also be charged a fee when claiming, so the losses will be significant for stakers.

// Get the unstaked service data
        reward = sInfo.reward;
        uint256[] memory nonces = sInfo.nonces;
        address multisig = sInfo.multisig;

        // Clear all the data about the unstaked service
        // Delete the service info struct
        delete mapServiceInfo[serviceId];

        // Update the set of staked service Ids
        // If the index was not found, the service was evicted and is not part of staked services set
        if (inSet) {
            setServiceIds[idx] = setServiceIds[setServiceIds.length - 1];
            setServiceIds.pop();
        }

        // Transfer the service back to the owner
        // Note that the reentrancy is not possible due to the ServiceInfo struct being deleted
        IService(serviceRegistry).safeTransferFrom(address(this), msg.sender, serviceId);

        // Transfer accumulated rewards to the service multisig
        if (reward > 0) {
            _withdraw(multisig, reward);
        }

↑ As you can see above, the rewards referenced are the once that were set in the snippet above, where the division by totalRewards happens in a calculation.

Recommendation

Consider implementing the following suggested pattern to support fee-on-transfer tokens adequately.

This is a common example:

    // Function to safely transfer FoT tokens accounting for the transfer fee
    function safeTransfer(address recipient, uint256 amount) public {
        uint256 balanceBefore = feeOnTransferToken.balanceOf(address(this));
        require(feeOnTransferToken.transfer(recipient, amount), "Transfer failed");
        uint256 balanceAfter = feeOnTransferToken.balanceOf(address(this));
        uint256 actualTransferAmount = balanceBefore - balanceAfter;

        // Logic to update internal accounting based on actualTransferAmount
        // This can include updating user balances, total supply calculations, etc.
    }

Applying this patter to the StakingToken code looks like this:

    /// @dev Deposits funds for staking.
    /// @param amount Token amount to deposit.
    function deposit(uint256 amount) external {
+       uint256 balanceBefore = stakingToken.balanceOf(address(this));

        // Add to the overall balance
        SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);

+       uint256 balanceAfter = stakingToken.balanceOf(address(this));

+       uint256 amountAfterOptionalFeeChargements = balanceAfter - balanceBefore;

        // Add to the contract and available rewards balances
-       uint256 newBalance = balance + amount;
+       uint256 newBalance = balance + amountAfterOptionalFeeChargements;
-       uint256 newAvailableRewards = availableRewards + amount;
+       uint256 newAvailableRewards = availableRewards + amountAfterOptionalFeeChargements;

        // Record the new actual balance and available rewards
        balance = newBalance;
        availableRewards = newAvailableRewards;

        emit Deposit(msg.sender, amount, newBalance, newAvailableRewards);
+       // ↑ no need to emit amountAfterOptionalFeeChargements instead (?)
    }

References

• https://github.com/code-423n4/2024-05-olas/blob/main/registries/contracts/staking/StakingToken.sol#L117-L125

Assessed type

ERC20

[c4-judge]

0xA5DF marked the issue as satisfactory