Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

contracts/linkStaking/test/OperatorVCSMock.sol

kp8877

The Solidity code for the OperatorVCSMock contract looks generally well-structured, but there are a few potential vulnerabilities and areas for improvement. Below are some key points to consider:

1. Lack of Access Control

  • Vulnerable Functions: Functions like addVault, removeVault, and setWithdrawalPercentage can be called by any user because there’s no access control implemented. This could lead to unauthorized changes to the vault or the withdrawal percentage.

  • Recommendation: Use an access control mechanism, such as the Ownable contract from OpenZeppelin, to restrict access to sensitive functions.

2. ERC20 Transfer and Approval Risks

  • Transfer Failures: The transferFrom and safeTransfer functions can fail if the token doesn't have sufficient allowance or balance, and it could result in the transaction reverting. However, using SafeERC20 helps mitigate the risk of incorrectly handling token transfers.

  • Recommendation: Always ensure that users have approved sufficient tokens before calling deposit. You can also implement checks and revert messages to provide more clarity on failure points.

3. Token Approval for Vault

  • Infinite Approval: The contract uses token.approve(_vault, type(uint256).max) which grants unlimited allowance. If the vault address is ever compromised, it could result in token loss.

  • Recommendation: Consider using a more controlled approval pattern. For example, set an approval limit that matches the expected usage, or revoke approvals when they are no longer needed.

4. No Event Emissions

  • Lack of Events: Functions that change state (like deposit, withdraw, addVault, and removeVault) do not emit any events. This makes it difficult to track on-chain activities and can hinder the debugging process.

  • Recommendation: Emit events for critical actions to improve transparency and auditability of contract activities.

5. Integer Overflow/Underflow

  • Potential Risks in Calculations: Although Solidity 0.8.x has built-in overflow and underflow checks, it’s still a good practice to ensure that your calculations are safe.

  • Recommendation: Be cautious of division and ensure that _amount is always a non-zero value in withdrawOperatorRewards to avoid division by zero.

6. Unbond Functionality

  • No Return Value: The unbond() function does not return any value or provide feedback. It is generally a good practice to indicate success or failure when performing operations.

  • Recommendation: Return a boolean value indicating success, or emit an event after a successful unbond.

7. Potential Gas Limit Issues

  • Function Complexity: Depending on the implementation of vault methods, the functions can have varying gas costs which might lead to out-of-gas exceptions if not properly managed.

  • Recommendation: Review the methods of IOperatorVault to ensure they are gas efficient and handle edge cases properly.

Example Code with Improvements

Here’s an updated version of the contract that incorporates some of the suggestions:

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.15;
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/access/Ownable.sol"; // Added for access control
​
import "../interfaces/IOperatorVault.sol";
​
/**
* @title Operator VCS Mock
* @notice Mocks contract for testing
*/
contract OperatorVCSMock is Ownable {
using SafeERC20 for IERC20;
​
IERC20 public token;
uint256 public operatorRewardPercentage;
uint256 public withdrawalPercentage;
IOperatorVault public vault;
​
event VaultAdded(address indexed vault);
event VaultRemoved();
event WithdrawalPercentageUpdated(uint256 newPercentage);
event Deposited(address indexed user, uint256 amount);
event Withdrawn(address indexed user, uint256 amount);
​
constructor(address _token, uint256 _operatorRewardPercentage, uint256 _withdrawalPercentage) {
token = IERC20(_token);
operatorRewardPercentage = _operatorRewardPercentage;
withdrawalPercentage = _withdrawalPercentage;
}
​
function deposit(uint256 _amount) external {
token.safeTransferFrom(msg.sender, address(this), _amount);
vault.deposit(_amount);
emit Deposited(msg.sender, _amount);
}
​
function withdraw(uint256 _amount) external {
vault.withdraw(_amount);
token.safeTransfer(msg.sender, _amount);
emit Withdrawn(msg.sender, _amount);
}
​
function unbond() external {
vault.unbond();
emit VaultRemoved(); // Consider emitting if appropriate
}
​
function withdrawOperatorRewards(address _receiver, uint256 _amount) external returns (uint256) {
require(_amount > 0, "Invalid amount");
uint256 withdrawalAmount = (_amount * withdrawalPercentage) / 10000;
return withdrawalAmount;
}
​
function updateDeposits(uint256 _minRewards, address _rewardsReceiver) external returns (uint256, uint256, uint256) {
return vault.updateDeposits(_minRewards, _rewardsReceiver);
}
​
function addVault(address _vault) external onlyOwner {
vault = IOperatorVault(_vault);
token.approve(_vault, type(uint256).max);
emit VaultAdded(_vault);
}
​
function removeVault() external onlyOwner {
vault.exitVault();
emit VaultRemoved();
}
​
function setWithdrawalPercentage(uint256 _withdrawalPercentage) external onlyOwner {
withdrawalPercentage = _withdrawalPercentage;
emit WithdrawalPercentageUpdated(_withdrawalPercentage);
}
}

Conclusion

Make sure to thoroughly test the contract and consider potential edge cases when integrating it with other contracts or systems. Implementing robust access controls and events will greatly enhance the contract's security and usability.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.