6.1 Improper access control management of Refiner when DLP Ownership is transferred
In addRefiner(...) function, the owner is set to the current owner of the dlpId, but if the owner is transferred then Refiner will still correspond to the old owner. This is an issue given functions addPermission(...) and updatePermissionApproval from the Query Engine uses the owner field of the Refiner struct.
Recommendation
Consider updating Refiner ownership whenever there is a DLP ownership transfer. This behavior should also be properly documented and communicated to DLP owners.
6.2 Native token can be lost when depositing an ERC20 token
In the deposit(...) function of the ComputeEngineImplementation contract, it is possible to deposit with a nonzero msg.value given the function is marked payable while using an ERC20 token. In that case, the contract accepts native asset funds, but the deposit will be registered under the token. As a result, the function will attempt to transfer that token from the user to itself and then to the treasury, which would be double paying. The native asset is not tracked in this case and can be lost.
Recommendation
Consider adding a check if the token is VANA, then the msg.value must be zero.
6.3 Inconsistent casting of maxTimeout
In the internal _registerJob (...) the passed parameter maxTimeout is of type uint256. However, in functions submitJob(...) and submitJobWithTee(...) where _registerJob(...) is called, the maxTimeout is passed as a uint80 data type. For consistency, the project should consider changing the maxTimeout type in _registerJob(...) to a uint80 and avoid silent casting.
Recommendation
Consider skipping the iteration for duplicate accounts to avoid double counting.
6.4 Owner address existence check can be optimized to save gas
In _registerJob() function, the instructions registry is queried just to check the owner's address as seen below:
function _registerJob(
uint256 maxTimeout,
bool gpuRequired,
uint256 computeInstructionId
) internal whenNotPaused returns (uint256 jobId) {
IComputeInstructionRegistry.ComputeInstructionInfo memory computeInstruction = instructionRegistry.instructions(
computeInstructionId
);
........
.......
This can be optimized further by adding a simple existence check directly in the instruction registry.
Recommendation
Consider directly adding a simple existence check in the instruction registry to optimize the code and save gas.
6.5 Inaccurate error emission
When updating job status via updateJobStatus(...) function, the contract checks the current job status if it is completed or Failed. If this is true, JobAlreadyDoneOrCanceled() error is emitted. The same inconsistency exists in cancelJob(...) function where the error of canceling a job is JobAlreadyDoneOrCanceled() but the code logic covers done, canceled and failed cases. Similarly, in _resubmitJobWithTee (...) the error message JobAlreadyDoneOrCanceled() would be incorrect if job.status is Submitted.
Recommendation
Consider ensuring consistent error emission with code logic.
6.6 Manual Initialization of TeePoolFactory could cause transaction revert when attempting bulk update
After the TeePool is deployed via the beacon proxy, the user with maintainer role must manually set the teepoolFactory address without which, maxTimeout updates would fail. This could cause reverts when attempting to bulk update.
function updateTeePoolProxyFactory(
ComputeEngineTeePoolProxyFactory newTeePoolProxyFactory
) external override onlyRole(MAINTAINER_ROLE) {
teePoolProxyFactory = newTeePoolProxyFactory;
}
Recommendation
Consider setting the teepoolFactory address when initializing the contract.
6.7 The owner privileges can be permanently lost when updating contract custodian role
When updating the contract custodian, there is no check to ensure the newCustodian is not the owner's address.
function updateCustodian(address newCustodian) external override onlyRole(DEFAULT_ADMIN_ROLE) {
_revokeRole(DEFAULT_ADMIN_ROLE, custodian);
custodian = newCustodian;
_grantRole(DEFAULT_ADMIN_ROLE, newCustodian);
}
Without this check, it is possible to set the custodian as the owner's address, effectively leaving the contract with only one custodian. If the custodian is changed again, the owner could lose privileges permanently.
Recommendation
Consider adding a check preventing the contract owner from becoming the contract's custodian.
6.8 Inconsistent key to value pair in _jobPayments mapping
In ComputeEngineStorageV1 contract, the mapping _jobPayments has a mapping key provider seen as mapping(uint256 jobId => mapping(address provider => PaymentInfo paymentInfo)) internal _jobPayments;. However, the implementation logic in _executePaymentRequestFromQueryEngine (...) uses the token address as the key for this slot and then a token address again within a mapping from the resulting struct.
function _executePaymentRequestFromQueryEngine(address token, uint256 amount, bytes calldata metadata) internal {
...........
...........
PaymentInfo storage paymentInfo = _jobPayments[jobId][token];
paymentInfo.paidAmounts[token] += amount;
paymentInfo.payer = jobOwner;
emit PaymentExecuted(jobId, token, amount);
computeEngineTreasury.transfer(msg.sender, token, amount);
}
A token is not a provider. This inconsistency should be addressed by changing the provider to another mapping key.
Recommendation
Consider updating the provider to another key in the mapping.
6.9 Inconsistent code logic with code comments
In DataRegistryImplementation contract, the code comments within addRefinementWithPermission(...) permission states: "The permission for an account is not allowed to be changed once set".
function addRefinementWithPermission(
uint256 fileId,
uint256 refinerId,
string calldata url,
address account,
string calldata key) external override whenNotPaused onlyRole(REFINEMENT_SERVICE_ROLE) {
...........
...........
// @dev Add permission for the account to access the refinement.
// The permission for an account is not allowed to be changed once set,
// to prevent previous refinements from being inaccessible.
if (bytes(_file.permissions[account]).length == 0) {
_file.permissions[account] = key;
emit PermissionGranted(fileId, account);
}
}
However, the addFilePermission (...) function allows the account's permission to be updated.
Recommendation
Consider either updating the code logic, reflecting the code comments, or updating the code comments to reflect the code logic.
Resolution
This is acknowledged.
