Vana

Core

6.1 Anyone can steal contributor rewards from the DLP

function addProof is an external function that allows any one to submit a proof for a fileId. With a valid proof, the owner of the file can claim contributor rewards from the DataliquidityPool by calling function requestReward.

function addProof(uint256 fileId, Proof memory proof) external override whenNotPaused {
    uint256 cachedProofCount = ++_files[fileId].proofsCount;
    _files[fileId].proofs[cachedProofCount] = proof;
    emit ProofAdded(fileId, cachedProofCount);
}

An attacker can potentially drain the DataliquidityPool ’s contributor rewards bucket by executing the below strategy.

As a first step of strategy, Attacker goes through the normal flow and adds a new file to the dataRegistry. He then requests for attestation of the file to the TeePool by paying the necessary fee. The proof will be generated by the TeePool which is submitted to the DataRegistry for the file. A good quality file will ensure that the reward size is large due to good score which increase the reward amount. Now, the attacker call the nominated DLP’s requestReward() to claim the contributor reward.

function requestReward(uint256 fileId, uint256 proofIndex) external override whenNotPaused nonReentrant {
    IDataRegistry.Proof memory fileProof = dataRegistry.fileProofs(fileId, proofIndex);
    //...
    bytes32 _messageHash = keccak256(
        abi.encodePacked(
            registryFile.url,
            fileProof.data.score,
            fileProof.data.dlpId,
            fileProof.data.metadata,
            fileProof.data.proofUrl,
            fileProof.data.instruction
        )
    );
    address signer = _messageHash.toEthSignedMessageHash().recover(fileProof.signature);
    if (!teePool.isTee(signer)) {
        revert InvalidAttestator();
    }
    //..
}

After completing the normal flow, the attacker now has a valid proof from the previous transaction. The attacker can start adding the new files to the DataRegistry and directly submit the above proof to each of the files by calling addProof on the DataRegistry for each of the files added. Attacker can then start claiming rewards from the DLP using the unique fileId and the proof to claim the contributor rewards.

Doing this strategy, attacker can potentially steal all of the contributor rewards in the DLP.

NOTE: Because of the restriction where DLP smart logic can’t really be changed our recommendation no longer is applicable, so your own fix approach can be used instead.

Recommendation

The _messageHash used for verifying if the signer is an activeTee does not include the fileId, and hence can be validated for all files. Adding fileId to _messageHash will prevent reusing of the proof against other files. Also, consider restricting the addProof to be called by valid tees only.

Resolution

Client fixed the issue by making the file Url unique in the data registry. This will ensure the same proof cannot be used across other files as url is part of the hash message while verifying the signer.

6.2 DLPRoot contract does not accept native tokens

The DLPRootImplementation contract allows any one to participate in the protocol ecosystem by staking vana tokens. The participants can be a simple staking user or can be a DLP. It rewards the participants with vana tokens using an epoch reward system. In order to participate as user or as a DLP, the contract has functions to accept vana tokens for staking and withdrawing the staked tokens.

function registerDlp(
    DlpRegistration calldata registrationInfo
) external payable override whenNotPaused nonReentrant {
    _createEpochsUntilBlockNumber(block.number);
    _registerDlp(registrationInfo);
}
function createStake(uint256 dlpId) external payable override nonReentrant whenNotPaused {
    _createEpochsUntilBlockNumber(block.number);
    _createStake(msg.sender, dlpId, msg.value);
}

To reward the participants with vana tokens, the DLPRootImplementation contract should have a mechanism for inflow of vana tokens. As vana tokens are the native tokens, there should be a receive/fallback function in the contract to support adding tokens for reward distribution. The receive function is missing from the DLPRootImplementation contract.

Recommendation

Add a receive function to the DLP root contract.

6.3 _msgSender should be used instead of msg.sender

When supporting Metatransactions for a contract, defining how the caller address is resolves is critical to the proper functioning of normal as well as metatransactions. During normal transaction, msg.sender is the actual caller. But, during Metatransaction, the msg.sender is the forwarder’s address, while the actual caller only known to the forwarder.

The ERC-2771 standard for meta transactions abstracts out these difference in the caller address and provides _msgSender() function to resolve the actual caller address.

The issue arises due to use of msg.sender in the below functions of the DLPRootImplementation contract. As a result, each of these functions will exhibit a different behaviour based on whether it is a normal transaction or a metatransaction.

Modifier onlyDlpOwner used in updateDlp and deregisterDlp so these will revert in multicalls

modifier onlyDlpOwner(uint256 dlpId) {
     if (_dlps[dlpId].ownerAddress != msg.sender) {
         revert NotDlpOwner();
     }
     _;
 }

Function createStake multicall stake creations will always belong to the trustedforwarder

function createStake(uint256 dlpId) external payable override nonReentrant whenNotPaused {
      _createEpochsUntilBlockNumber(block.number);
      _createStake(msg.sender, dlpId, msg.value);
  }

Function closeStakes a stake opened by user via regular call, impossible to close stake via multicall

function closeStakes(uint256[] memory stakeIds) external override nonReentrant whenNotPaused {
     _createEpochsUntilBlockNumber(block.number);
     for (uint256 i = 0; i < stakeIds.length; i++) {
         _closeStake(msg.sender, stakeIds[i]);
     }
 }

Function withdrawStakes a stake closed by a user via regular call, impossible to close via multicall

function withdrawStakes(uint256[] memory stakeIds) external override nonReentrant whenNotPaused {
        _createEpochsUntilBlockNumber(block.number);
        for (uint256 i = 0; i < stakeIds.length; i++) {
            _withdrawStake(msg.sender, stakeIds[i]);
        }
    }

Recommendation

Replace msg.sender with _msgSender() provided by ERC-2771 standard. This will ensure, the resolving of the actual call is always delegated to ERC-2771 standard which knows how to resolve the actual caller address.

Resolution

Fixed

6.4 Removing a TEE can prevent a reward from being claimed

Contributor reward can claimed for the file after the proof is submitted by the assigned tee. In order to claim the contributor reward, function requestReward is invoked with fileId and proofIndex. The function computes the eligible reward amount for the contributor and transfers it. While processing the rewards, the function check if the signer of proof is a tee.

function requestReward(uint256 fileId, uint256 proofIndex) external override whenNotPaused nonReentrant {
    IDataRegistry.Proof memory fileProof = dataRegistry.fileProofs(fileId, proofIndex);
    //...
    address signer = _messageHash.toEthSignedMessageHash().recover(fileProof.signature);
    if (!teePool.isTee(signer)) {
        revert InvalidAttestator();
    }
    //...
}

Refer to the below codesnippet where an address is recognised as tee only if it is currently active.

function isTee(address teeAddress) external view override returns (bool) {
    return _tees[teeAddress].status == TeeStatus.Active;
}

Consider a case where the proof was submitted by a tee, but was removed by the maintainer role. In such cases, if the rewards were not claimed before the removed of tee, then such rewards cannot be claimed.

Recommendation

As jobs can be processed by active tees, post the proof is submitted, as long as tee is listed in _teeList, it should be safe to assume, the tee was a valid attestator for the proof. This will ensure that contributor can claim rewards even when tees are removed from active tee list.

Resolution

Not fixed, we are ok with this edge case

6.5 TeePool’s implementation for pausing and unpausing contract is not effective

TeePoolImplementation contract derives from PausableUpgradeable library of openzeppelin to support pausing of the contract incase of an unexpected situation and unpause it when the situation is resolved.

The pausing and unpausing needs to be enabled at external function level in the implementation contract to enable and disable functionality to mitigate the risk and hence needs to be implemented by the derived contract using the whenNotPaused and whenPaused modifiers.

These modifiers should be assigned to external functions of the contract to enable or disable when the contract is paused.

In the TeePoolImplementation contract, these modifiers are not attached to any of the functions. Hence, even though the pause is called, it will not be effective.

Recommendation

Attach the whenNotPaused and whenPaused modifiers to enable and disable functions during pause.

Resolution

Fixed

6.6 Blocking users in DAT token contract does not prevent from voting or delegation

DAT token contract supports blocking transfer funds of funds if the sender is on the blocked list. The function _update checks for the sender in the blockList, if found will revert the transaction. But since the block list only restricts the sender, it is possible for someone to send tokens to a user on blocked list.

function _update(
        address from,
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) whenNotBlocked(from) {
        super._update(from, to, amount);
}
modifier whenNotBlocked(address from) {
    if (_blockList.contains(from)) {
        revert UnauthorizedUserAction(msg.sender);
    }
    _;
}

As DAT token is also a governance tokens, hence holders will have voting rights. Blocked holders will still have full voting powers including delegation to another address. DAT token contract does not implement restrict the blocked holders from participating in voting or delegation.

Recommendation

Override the votes contract related functions to apply the restriction.

Resolution

Fixed

6.7 Changing the ProofInstruction in DLP could prevent processing the reward request

The proof for the file is added by the Active Tee validator as part of Job processing In the Tee Pool implementation contract. When the requestReward(...) function is called on DLP, the file proof information is read from the data registry.

function requestReward(uint256 fileId, uint256 proofIndex) external override whenNotPaused nonReentrant {
      IDataRegistry.Proof memory fileProof = dataRegistry.fileProofs(fileId, proofIndex);
      if (keccak256(bytes(fileProof.data.instruction)) != keccak256(bytes(proofInstruction))) {
          revert InvalidProof();
      }

In DLP, the owner has ability to update the proofInstruction state variable by calling updateProofInstruction(...)

function updateProofInstruction(string calldata newProofInstruction) external override onlyOwner {
       proofInstruction = newProofInstruction;
       emit ProofInstructionUpdated(newProofInstruction);
   }

But, updating the proofInstruction state variable could cause a revert for proof already submitted by the Tee Pool but, reward request were not processed. The revert will happen in the requestReward(...) function where it compares the state variable with the fileProof.data.instruction field in the fileProof already submitted.

When they dont match, the call will revert with InvalidProof error.

NOTE: You don’t need to provide a fix for this one. During the meeting we had discussed how adding logic to refunds users in this very rare case would add complexity. Instead it is the responsibility of the DLP entity to reimburse the user offchain should this happen.

Resolution

This is unlikely to occur and in the case that it does, the DLP entity itself should reimburse the user offchain.

6.8 DLP owner can withdraw their stake and continue running the DLP

While registering as a new DLP, the DLP owner needs to provide stake greater than or equal to the minDlpRegistrationStake. As the DLP attracts more users to stake their tokens, the DLP could get listed into the rewards eligible list resulting in rewards. At this point, the owner of DLP could close and withdraw their staked amount and continue to operate as DLP owner. This allows the DLP owner to operate multiple DLPs while reducing their risk.

Recommendation

Consider adding a flag to stake storage data that indicates whether it is a "registration stake", and special logic preventing closure of a stake while it’s still active can occur in this case. Alternatively, if the risk of DLP registrants closing their stakes is determined to be negligible by the Vana team this finding can be acknowledged.

Resolution

Not fixed, we are ok with this scenario

6.9 DLP registration can be frontrun

As the economic commitment to register the DLP is low, it is possible that a bad actor can front run the registration of DLP and claim ownership of the DLP address. Once the DLP address is added the dlpIds mapping, that address cannot be used for registration leading to DOS for the real owner.

if (dlpIds[registrationInfo.dlpAddress] != 0) {
    revert InvalidDlpStatus();
}

DLP’s registration with low registration stake amount and also the ability for the owner to claim and withdraw registration stake any time opens up this vulnerability and hence it could be abused by bad actors effecting the protocol key participant.

As the DLP address is obtained post deployment, if a bad actor claims the Dlp address and configure owner and treasury addresses, the rewards and management of DLP will be in the control of the bad actor.

Recommendation

As the intention of low registration stake amount was to encourage more participation, the recommendation is to lock the registration stake amount for longer periods. This will discourage the bad actors to participate in such abuse.

If a "get owner" function were exposed in the DLP contract, then it could allow for a check during registration in the root contract that the caller address is the owner of the DLP which would prevent this issue, an example code snippet is shown below:

require(IDataLiquidityPool(registrationInfo.dlpAddress]).ownerAddress==msg.sender,"Not DLP Owner");

Resolution

Not a problem as the DLP creators should register their DLP in the very next step after deployment

6.10 Eligible DLP list should be rotated based on staked amounts on periodic basis

The DLP Root contract intends to reward the top DLPs based on staking. That means amounts staked into DLPs defines the _eligibleDlpsList list. Those who have the highest stake are the ones who should be getting the rewards for the subsequent epochs.

Lets assume a scenario where _eligibleDlpsList is operating at max limit, say 16 DLPs are already identified. At this point, if a new DLP is registered with a stake amount exceeding the amount held by one of the members in the _eligibleDlpsList, the new DLP should be part of the list.

But, since the _eligibleDlpsList is operating at max limit, the new DLP is not considered for the _eligibleDlpsList even when the staked amount was higher. The new DLP will get the opportunity for rewards only if one of the DLP gets disqualifed due to unstaking or unregister. This approach of updating the _eligibleDlpsList list might discourage some of the new DLPs to come onboard and participate in the ecosystem.

This behavior can potentially be abused by a malicious actor by registering many new DLPs until the eligibleDlpsLimit is met, at which point any new genuine DLP owners who attempt to register will not be added to the eligible list. This type of attack has an economic cost for the actor since they must provide a stake for each registration

Recommendation

Consider implementing a mechanism to remove the a DLP with the lowest computed stake amount from _eligibleDlpsList when a different DLP recieves a stake that is greater than the existing lowest computed stake DLP.

Resolution

We have a way to rotate Eligible DLPs by updating the dlpEligibilityThreshold (+ a small stake for the DLP that are above the dlpEligibilityThreshold to update the status)

6.11 Incorrect rewards calculation in estimatedDlpRewardPercentages

The DLP Reward estimates are readable using estimatedDlpRewardPercentages(...) function for a given list of DLPs. The DLPs are provided as input by the caller, and based on the function logic, the estimated reward amount for each DLP is returned. There are two issues identified in the logic:

1. The topDlps fetched for this purpose is hard coded as 16, where instead the eligibleDlpsLimit should be used to consider all eligible DLPs; 2. Reward should be returned for only top DLPs and for others, the reward should be 0;

Refer to the below code snippet where the logic loops over dlpIds provided as input by the caller to compute the estimated reward for each DLP. The function does check if each DLP is in the topDlp and a boolean variable isInTop is set to true in this case, however this variable is left unused after being set. Rewards are given to all DLPs regardless of whether they are considered a top DLP or not, as shown below:

function estimatedDlpRewardPercentages(
    uint256[] memory dlpIds
) external view override returns (DlpRewardApy[] memory) {
    // Hardcoded to use the top 16 DLPs, should be the size of the eligible DLPs set
    uint256[] memory topDlps = topDlpIds(16);
    //...
    for (uint256 i = 0; i < dlpIds.length; i++) {
        //...
        // Check if DLP is in top 16
        for (uint256 j = 0; j < topDlps.length; j++) {
            if (topDlps[j] == dlpId) {
                isInTop = true;
                break;
            }
        }
        // Regardless of `isInTop` rewards are distributed anyways
        // If DLP is not in the `topDlps` array they still get rewards
        uint256 rewardAmount = (dlpStake * epochRewardAmount) / totalStakeAmount;
        uint256 stakeReward = (((100e18 * rewardAmount) / dlpStake) * stakersPercentage) / 1e20;
        //...
    }
    return result;
}

As such, even dlps that are not in the top Dlp list are estimated to have a non zero reward amount. This looks incorrect.

Recommendation

Consider adding a check to ensure the DLP has the value isInTop set to true before assigning values to rewardAmount and stakeAmount, this will ensure that rewards are only given to the correct DLPs. For the topDlpIds() function call consider getting all eligible DLPs instead of using a hardcoded value.

Resolution

We want to estimate the APY for all DLPs that are eligible even though they are not in top 16

6.12 Treasury’s reward in an epoch can be lost

As the new epochs are created for a block, the previous epochs are finalised through _finalizeEpoch(...) function. The _finalizeEpoch(...) computes the reward for each of the DLP that participated in the epoch and credits the reward amount for each DLP respectively.

Each DLP can configure there stakersPercentage. Any reward left is transferred to the DLP’s treasury address. The issue lies in the below code snippet, where the return value for the low level call function is not being checked.

(bool success, ) = dlp.treasuryAddress.call{
                 value: (epochDlp.rewardAmount * (100e18 - epochDlp.stakersPercentage)) / 100e18
           }("");
//skip this in phase 1 to avoid reverting the whole transaction when the treasury transfer fails
//                if (!success) {
//                    revert TransferFailed();
//

Incase the call fails, it will be ignored. As a result of this, the DLP’s treasury will not receive the reward for the epoch. The contract does not have a function for the treasury to claim the rewards that did not succeed during the _finalizeEpoch(...) function. As a result, the treasury rewards are locked in the DLPRoot and will not be available for DLP to claim.

Recommendation

The recommendation is to track and credit the DLP treasury rewards in a separate mapping. Provide a function for the DLP treasury to claim those rewards.

Resolution

We now save which DLP successfully received the reward and, if there are any errors, we will send the reward manually from the treasury

6.13 DLP eligibility and sub-eligibility threshold invariant is not enforced

The logic in the DLPRootImplementation surrounding the eligibility and sub-eligibility thresholds are designed in a way that assume that the eligibility threshold is always greater than the sub-eligibility threshold. However, in the setter functions updateDlpEligibilityThreshold and updateDlpSubEligibilityThreshold there are no checks to ensure that this invariant is held.

Recommendation

Consider adding checks to updateDlpEligibilityThreshold and updateDlpSubEligibilityThreshold to ensure that the sub-eligibility threshold is always lower than the eligibility threshold.

Resolution

Fixed

6.14 Epoch storage field isFinalised inconsistently utilized

The struct Epoch is used to track information about an epoch, and the mapping _epochs uses epoch IDs as a key to be able to track historical information about all epochs that have occurred. One of the fields in the Epoch struct is the boolean isFinalised which is used inconsistently by the DLPRootImplementation contract.

struct EpochInfo {
    uint256 startBlock;
    uint256 endBlock;
    uint256 reward;
    bool isFinalised;
    uint256[] dlpIds;
}

When the DLP root contract is being initialized, a "zero epoch" is created and immediately set as finalized. However, all other epochs that are generated through _createEpochsUntilBlockNumber and finalized through _finalizeEpoch will never have their isFinalised bit set to true. When querying information about epochs through the external view function epochs, only the "zero epoch" will be considered finalised, and it is not possible for any other "natually generated" epoch to be finalised, even once it has been passed through the function _finalizeEpoch.

Recommendation

Consider setting epochs as finalized through the function _finalizeEpoch.

Resolution

Fixed

6.15 Epoch to claim should be bound in claimStakeRewardUntilEpoch

The DLPRootImplementation contract provides an external function to claim rewards up to a user specified epoch number by calling the claimStakeRewardUntilEpoch(...) function.

function claimStakeRewardUntilEpoch(...) external nonReentrant whenNotPaused {
    _createEpochsUntilBlockNumber(block.number);
    _claimStakeRewardUntilEpoch(stakeId, lastEpochToClaim);
}

The problem arises when a user passes lastEpochToClaim parameter with a value greater than the current epoch.

function _claimStakeRewardUntilEpoch(uint256 stakeId, uint256 lastEpochToClaim) internal {
    // ...
    while (epochToClaim > 0 && epochToClaim <= lastEpochToClaim) {
        Epoch storage epoch = _epochs[epochToClaim];
        EpochDlp storage epochDlp = epoch.dlps[stake.dlpId];
        if (
            epochToClaim == 0 ||
            epochDlp.totalStakesScore == 0 ||
            (stake.endBlock > 0 && epoch.endBlock > stake.endBlock)
        ) {
            break;
        }
        uint256 stakeScore = calculateStakeScore(stake.amount, stake.startBlock, epoch.endBlock);
        // ...
    }
    // ...
}

When the lastEpochToClaim is greater than epochsCount, then the above logic will read an empty epoch from the _epochs collection. As the epoch is uninitialized, epoch.endBlock will hold a value of 0. So, when the stakeScore is computed in calculateStakeScore(...) function, it will revert while computing the daysStaked as stakeStartBlock will have a value greater than 0 and blockNumber will be 0 as its value.

function calculateStakeScore(
    uint256 stakeAmount,
    uint256 stakeStartBlock,
    uint256 blockNumber
) public view returns (uint256) {
    uint256 daysStaked = (blockNumber - stakeStartBlock) / daySize;
    return (stakeAmount * _getMultiplier(daysStaked)) / 100;
}

Recommendation

Consider adding a validation in claimStakeRewardUntilEpoch to restrict the lastEpochToClaim to be less than epochsCount state variable. Alternatively, limit it to epochsCount - 1 incase it was larger value.

function claimStakeRewardUntilEpoch(
    uint256 stakeId,
    uint256 lastEpochToClaim
) external nonReentrant whenNotPaused {
    _createEpochsUntilBlockNumber(block.number);
    uint256 maxEpoch = Math.min(lastEpochToClaim, epochcount-1);
    _claimStakeRewardUntilEpoch(stakeId, maxEpoch);
}

Resolution

Fixed

6.16 Fetching epochs for a DLP includes epoch0

The function dlps(...) returns the information about the DLP based on the dlpId. The logic of reading the epochs for the DLP also returns Epoch 0 for every call. Refer to the below code snippet where memory array for dlp.epochIdsCount + 1 size is created and all epoch ids applicable for the DLP are assigned to the array.

uint256[] memory epochIds = new uint256[](dlp.epochIdsCount + 1);
for (uint256 i = 1; i <= dlp.epochIdsCount; i++) {
    epochIds[i] = dlp.epochIds[i];
}

Since the epochIds are assigned from index 1, the first element of the epochIds array will always remain empty with a value of zero which refers to Epoch 0.

Recommendation

Consider adjusting the loop logic to fill the array from index 0 rather than index 1 to avoid Epoch 0 from being referred to as the first element.

Resolution

Fixed

6.17 TotalStakesScore can be updated for partial list of DLPs

The TotalStakeScore value for each DLP plays an important role in the computation of reward for the stake amount. Hence, all the DLPs in the _eligibleDlpsList should be updated in one go every time the TotalStakeScore values are updated. The current implementation of saveEpochDlpsTotalStakesScore(...) function does not enforce the updation of all eligible DLPs in one transaction.

Recommendation

The recommendation is to ensure all _eligibleDlpsList are updated with their respective TotalStakeScore values in one call. To do that, add validation to ensure each of the DLP in the _eligibleDlpsList should be present in the input array.

Resolution

not fixed. it’s not a problem because the offline microservice might calculate the DLP scoring asynchronous and we want to upload the scoring of each DLP as soon as the scoring is calculated. There is no dependencies between DLPs.

6.18 Unnecessary use of checkpoints

The checkpoints for _stakeWithdrawalDelayCheckpoints and _rewardClaimDelayCheckpoints appear to be unnecessary given how they are used in the codebase. Only the latest entries are ever queried, no historical features of the checkpoint library are ever used. A standard uint256 storage variable can achieve the same while reducing complexity and gas costs. Both setters for these checkpoints feature event emission, meaning that even with standard storage variables it is possible to track the delay histories using off chain logging.

Recommendation

Consider using a standard uint256 storage variable instead of checkpoints for tracking the withdrawal and claim delay checkpoints.

Resolution

At some point we might want to use on unstake or claim the values that were active at stake.startData

6.19 Missing function to revoke permissions on a file in Data registry

In the DataRegistry contract the function addFilePermission can be used to set permissions for a file. If permissions need to be removed for some reason, this function can still be used to clear permission by setting the key to an empty string, however the event emission will state that this empty permission has been "added" which is counterintuitive from a UI perspective.

function addFilePermission(uint256 fileId, address account, string memory key) external override whenNotPaused {
    if (_msgSender() != _files[fileId].ownerAddress) {
        revert NotFileOwner();
    }
    _files[fileId].permissions[account] = key;
    emit PermissionGranted(fileId, account);
}

Recommendation

Consider adding logic to addFilePermission to detect when permissions are being removed and emit an event related to permission removal, or alternatively a function dedicated to removing permissions can be implemented.

Resolution

Not fixed, we fine with this

6.20 Reward calculation logic is duplicated in view and claiming flows

DLPRootImplementation contract offers a view function for users to view the claimable rewards, namely calculateStakeClaimableAmount(...) function. The contract also offers a claiming functions to claim the rewards based on stakeId and epochIds.

While the logic for computing the rewards is same for view and claiming flow, the code was implemented separately in each of the flows, essentially duplicating the same logic twice.

Recommendation

As a best practice, such shared logic should be placed in an internal function to be shared across the two flows. This will ensure consistency.This helps in case some areas of the code are changes, the changes will be reflected both in the state changing and calculation functions will also improve code readability, maintainability and structure of the logic.

Resolution

Fixed

6.21 Timestamp should indicate reward claim instead of reward amount

The function requestReward only allows rewards to be claimed once, by checking that the existing file rewardAmount is zero. If the rewardAmount is nonzero, then it means a reward has already been claimed. However, it is possible for a reward amount to be zero if the fileRewardFactor for a given DLP is zero. In this case, the function requestReward can be called any number of times, each time emitting an event and changing the reward claim timestamp. It is also possible to change the proof index used on each call. The function is shown below:

function requestReward(uint256 fileId, uint256 proofIndex) external override whenNotPaused nonReentrant {
    // ...
    File storage file = _files[fileId];
    if (file.rewardAmount != 0) {
        revert FileAlreadyAdded();
    }
    // ...
    file.timestamp = block.timestamp;
    file.proofIndex = proofIndex;
    file.rewardAmount = (fileRewardFactor * fileProof.data.score) / 1e18;
    // ...
    emit RewardRequested(registryFile.ownerAddress, fileId, proofIndex, file.rewardAmount);
}

Recommendation

Consider checking if a file is already added by using the file.timestamp instead of the file.rewardAmount to ensure that the function requestReward cannot be called multiple times while the fileRewardFactor is zero.

Resolution

Client agreed that using timestamp is a better way to check if the file was already rewarded. But, client will not fix for this release, will instead consider during the next release.

6.22 UnauthorizedUserAction event log does not relay complete information

In DAT token contract, while the moving the tokens between two accounts, the from address is checked to ensure it is not in the block list. In the case where the user is in the block list, the UnauthorizedUserAction revert message is used and the transaction fails.

The UnauthorizedUserAction event only records the caller address, but does not record the from address which was blocked. The event does not contain complete information of why the transaction was reverted.

In the case of a transferFrom call, where the caller is transferring assets from a blocked from address to some to address, the revert error message will use msg.sender which is the caller address, not the blocked from address which may be misleading.

modifier whenNotBlocked(address from) {
       if (_blockList.contains(from)) {
           revert UnauthorizedUserAction(msg.sender);
       }
       _;
   }

Recommendation

Consider passing the from address in addition to the caller for the revert message.

 // declaration
 error UnauthorizedUserAction(address account, address from);
// usage
 revert UnauthorizedUserAction(msg.sender,from);

Resolution

Fixed

6.23 Unnecessary payable operator in addProof

The TEE generates and submits the proof for the file associated with the job id. Since the TEE will receive the fee for attestation of the file, there is no need to have payable operator for addProof function.

function addProof(uint256 jobId, IDataRegistry.Proof memory proof) external payable override onlyActiveTee {
    //...
}

Recommendation

Remove payable operator from the function signature.

Resolution

Fixed

6.24 Unused MAINTAINER role in DataLiquidityPool

The role MAINTAINER is not used in the DLP contract, usually the maintainer role is assigned to handle certain operations specific to the contract such as pausing. But in the case of DLP these abilities are given to the admin instead.

This breaks the common convention of the maintainer role handling contract specific management functions, which is consistently applied in all other contract implementations (TeePool, DataRegistry, DAT, DLPRoot)

Recommendation

Consider adjusting the role required for relevant functions in the DataLiquidityPoolImplementation file to use the maintainer role instead of the admin role.

Resolution

Not fixed, we fine with this

6.25 Unused imports

Quick rough writing, can clean up later:

DataRegistryImplementation
    ECDSA import unused
    MessageHashUtils import unused
TeePoolImplementation
    PausableUpgradeable import but modifiers not used

Resolution

Fixed

Findings (Follow-up Metrics Review)

6.26 estimatedDlpRewardPercentages does not consider non-top DLPs

The DLP root metrics contract contains the function estimatedDlpRewardPercentages which determines the amount of rewards that some given set of DLPs should receive, for the latest epoch on the DLP root contract. To estimate rewards, the function first gets all top performing DLPs and calculates their total stake amount and ratings, before applying a pro-rata distribution of rewards based on the individual stake amounts and ratings of the DLPs provided in the dlpIds argument array.

When DLP IDs provided in the dlpIds argument array are all considered part of the "top DLPs" in the root contract then estimations will be correct. Rewards should only be given to top performing DLPs, however the estimate function does not check whether a provided DLP ID is in the topDlpsList.

This makes it possible to provide a non-top-performing DLP ID to the function and it will calculate a pro-rata reward based on the staked amount, even though it isn’t a top performing DLP and therefore should not be eligible for rewards.

Recommendation

This function is only used by DLPRoot.estimatedDlpRewardPercentages which is an external view function, and won’t have any on-chain impact. However, it should be noted that any off-chain entities that interact with this function may receive incorrect reward estimations for non-top-performing DLPs. This can be addressed with a check for each DLP in the dlpIds argument array and for non-top-performers their rewards can default to zero.

Resolution

We are aware of this behavior. The estimatedDlpRewardPercentages function is designed to estimate the APY even for DLPs that are not currently eligible. The intention is to provide an approximation of the APY for cases where a DLP may enter the top N DLPs in the current epoch.

6.27 Unnecessary receive function in DLPRootImplementation

With the addition of the stakes and reward treasuries for the DLPRootImplementation contract, funds can be sent to the treasuries directly, an all funds related to staking can be passed through the payable functions createStake and registerDlp. As a result, the receive function in the root contract is no longer necessary.

Recommendation

Consider removing the receive function in the DLPRootImplementation contract.

Resolution

Fixed