Sentient Labs: Merkle Staking & Treasury

Description

There are several locations that have hard-coded gas amounts:

• emergencyWithdraw in SentientRewardDistributor
• emergencyWithdraw in SentientStakingDepositManager

This is considered a bad practice in that gas costs can change or it might not be enough for smart contracts to utilize.

Recommendation

Consider forwarding all gas when making external calls

Description

On-chain code should have clear descriptions of what is does. That includes what compiler the developer intends the code to be compiled with so that future, valid but breaking compiler versions can be avoided.

Recommendation

Consider fixing the compiler version of ArtifactFactory.

Description

There are a few locations in the codebase where multiple functions provide the same functionality. Such code can make it harder to understand the features of a codebase or how it can be used.

• In ArtifactManager there are two functions that provide the active artifacts: getArtifactList and artifactList().
• In ArtifactManager there are wrappers around the AccessControl grantRole and revokeRole functions just for a role bespoke to this contract: grantFactoryRole and revokeFactoryRole.

Recommendation

Consider whether these superfluous functions are critical to the contract's lifecycle and remove them if not.

Description

The deposit function in SentientStakingDepositManager is payable to allow users to stake SENT tokens on artifacts. It also allows users to stake ERC20 tokens. It does not, however, check, that if a user is staking ERC20 that they have not accidentally sent SENT with the transaction.

Recommendation

Consider adding such a check to protect a user from creating such an unintended, yet valid transaction.

Description

If a merkle root was set just before a pause, then due to whenNotPaused modifier, authorized parties might not be able to deny it and window may pass before unpausing.

Given pauses are only executed during catastrophic times, and not allowing to deny roots during such periods might allow a malicious root being added.

for e.g:

• at t=t0, root was added
• at t=t1, pause was triggered
• now at t=t3 when sufficient time has passed, approve could be called (front-running deny if required)

Recommendation

• consider allowing denyMerkleRoot even if paused
• this could be tackled on operational front by batching pausing with increase merkleRootPendingPeriod to a duration larger than for which contracts will be paused for

Description

The reportArtifactUsage function in the ArtifactManager contract allows the holder of the ADMIN_ROLE to storage usage data for any artifact. There is an implicit assumption that the timestamp used should increase over time and we believe this should be made explicit. It would effect all reporting on artifact usage if it were ever set to a previous time.

Recommendation

Revert if the timestamp is less or equal to a previous timestamp.

Description

In the ArtifactManager contract artifacts usage is stored and retrievable by iterating through the stored details. However, there is a top-level variable stored called usageCount that denotes the total usage of an artifact over time. This means then, that the function _calculateTotalUsage function is entirely superfluous as it iterates through to calculate this stored variable.

Recommendation

Remove this function and retrieve the stored value when needed

Description

In the codebase there are several functions that emit events to update off-chain systems of on-chain state changes. Some of these do not guard against the possibility of emitting an event even though the on-chain state has not changed. For example:

• SentientRewardDistributor.setMerkleProofPendingPeriod
• SentientStakingDepositManager.setDepositEnabled
• SentientStakingDepositManager.setWithdrawEnabled
• ArtifactFactory.setArtifactBeacon
• ArtifactManager.setArtifactFactory

Recommendation

Check input parameters and revert if they are identical to the existing state they are assigning to

Description

There are several locations where the code can be improved in its accuracy or clarity

• The use of unchecked to increment loop counters in ArtifactManager when solidity now does this automatically as of version 0.8.22
• The use of abi.encodeWithSelector is not type safe and should be eschewed in favor or abi.encodeCall
• Contracts that inherit Context have _msgSender() and _msgData() to allow chain-specific unwrapping of these values. Therefore, a consistent style of using these over msg.sender or msg.data is preferred. ArtifactFactory, ArtifactManager, SentientRewardDistributor, and SentientStakingDepositManager all are inconsistent in this style.
• In SentientRewardDistributor, the SentientRewardDistributor__InvalidProofs error is unused
• In SentientStakingDepositManager, the StakingTokenId struct is unused
• In ArtifactManager, there are two functions named artifactList that return different pieces of information.
• In Artifact, the MAX_INITIAL_OWNERS constant is unused
• Both IArtifact and IArtifactManager define a ArtifactInfo struct which should be named differently as they represent different things
• Consider documenting that, in SentientRewardDistributor.claimRewards for keccak256(abi.encodePacked(msg.sender, amounts[i])); holds valid only if there is atmost 1 entry per address in when constructing root.
• Using codesize() in WSENT::withdraw provides minimal gas optimizations at the expense of readbility and transparency. Consider replacing it with 0x00 for readability or even use mainline WETH9
• The _artifactList state variable in the ArtifactManager contract is used to house only active artifacts. Its name however may mislead readers/developers to think that it houses all artifacts instead.
• The comments in Artifact's initialize function describe storage slots and packing that is incorrect. _owner is stored in its own slot and the ArtifactInfo struct stores _expectedApy and _commissionRate as uint256 so there's no packing with those two either.

Recommendation

Consider updating the highlighted sections above to be more accurate or clear in their intent.

Description

Adding a security contact into the smart contract allows good-faith actors to notify developers of issues in the code as quick as possible. Without such, it may be impossible for a found bug to be reported. None of the contracts in scope had such a security contact.

Recommendation

Consider adding a NatSpec comment containing a security contact at the top of the contract. Here is an instructive example from ethereum-lists.

Description

consider renaming to pendingMerkleProofs to pendingMerkleRoots to reflect its correct purpose.

Recommendation

- mapping(address => bytes32) public pendingMerkleProofs+ mapping(address => bytes32) public pendingMerkleRoots

- mapping(address => bytes32) public pendingMerkleProofsTimestamp+ mapping(address => bytes32) public pendingMerkleRootsTimestamp

- uint256 public merkleProofPendingPeriod = 1 hours;+ uint256 public merkleRootPendingPeriod = 1 hours;

Description

In SentientStakingDepositManager, the deposit function allows users to use native tokens for their staking. The withdraw function returns these deposits as wrapped tokens.

Recommendation

Consider allowing the user to withdraw native tokens from the contract.

Description

In the ArtifactManager contract, artifact reporting is retrieved using several functions. Though many of these only work if the artifact is active. As these are view functions and developed to provide value on chain, it seems curious that this reporting distinction would be made as the data is not private in any way.

Recommendation

Consider whether inactive artifacts should be reported on as well.

Description

In the codebase there are several locations where loop variables are incremented using unchecked variables. This was a widespread practice as the Solidity Compiler would add expensive bounds checking in loops. But as of Solidity v0.8.22, the compiler no longer does this and instead uses the method as coded here. An overview of the feature is explained in the Solidity blog here.

Recommendation

Consider removing unchecked loop increments

Description

In SentientStakingDepositManager and SentientRewardDistributor, ReentrancyGuard can be switched with ReentrancyGuardTransient for consistent gas savings across most of the user transactions.

Recommendation

Consider switching to ReentrancyGuardTransient if transient opcodes are supported