Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: high
Invalid

contracts/vesting/Vesting.sol

The smart contract seems well-structured and leverages OpenZeppelin’s libraries, but here are some potential vulnerabilities, pitfalls, and optimizations to ensure security and robustness. Here's what I found:


1. Reentrancy Vulnerability in terminateVesting()

Even though OpenZeppelin's SafeERC20 protects against low-level call issues, reentrancy remains a risk if tokens implement malicious behavior (e.g., reentrant ERC777 tokens).

Vulnerability:

  • The function transfers tokens to the owner before marking the vestingTerminated state.

  • If a malicious ERC777 token allows reentrancy, it could re-enter terminateVesting() and cause unexpected behavior.

Mitigation:
Move vestingTerminated = true before the token transfers to prevent reentrancy.

vestingTerminated = true;
emit VestingTerminated();
for (uint256 i = 0; i < _tokens.length; ++i) {
address token = _tokens[i];
uint256 toWithdraw = IERC20(token).balanceOf(address(this)) -
vestedAmount(token, uint64(block.timestamp));
SafeERC20.safeTransfer(IERC20(token), owner(), toWithdraw);
}

2. Unchecked Arithmetic in Loop

The code uses ++i and arithmetic directly without safe math. Although Solidity 0.8+ has built-in overflow checks, explicit checks might still help in reducing risks and improving readability.

for (uint256 i = 0; i < _tokens.length; ++i) {

You can keep it as is since it relies on Solidity's overflow protections, but if performance or clarity is a concern, it may benefit from OpenZeppelin’s SafeMath (if handling token amounts safely is critical).


3. Unintentional Token Behavior Handling (Zero Balance Transfers)

If _tokens contains addresses of tokens that have zero balance, the call to SafeERC20.safeTransfer will still succeed, but this might confuse users or waste gas.

Mitigation:
Skip tokens with zero balance in the loop.

uint256 toWithdraw = IERC20(token).balanceOf(address(this)) -
vestedAmount(token, uint64(block.timestamp));
if (toWithdraw > 0) {
SafeERC20.safeTransfer(IERC20(token), owner(), toWithdraw);
}

4. Using block.timestamp

block.timestamp can be manipulated slightly by miners. In most cases, this manipulation is minimal, but in critical vesting logic, you might want to use block.number or at least be aware that timestamps can vary by ~15 seconds.

Suggested:

  • Keep using block.timestamp if strict timing precision is not essential.

  • If you want to be extra safe, ensure that timestamps have a reasonable buffer.


5. No OnlyBeneficiary Modifier for releaseRemaining()

Currently, anyone can call releaseRemaining(). Although it only works when vestingTerminated is true, you might still want to restrict access.

Mitigation:
Add a modifier to restrict the function to only the beneficiary.

modifier onlyBeneficiary() {
require(msg.sender == beneficiary(), "Caller is not beneficiary");
_;
}
function releaseRemaining(address _token) external onlyBeneficiary {
if (!vestingTerminated) revert VestingNotTerminated();
uint256 toRelease = IERC20(_token).balanceOf(address(this));
emit ERC20Released(_token, toRelease);
SafeERC20.safeTransfer(IERC20(_token), beneficiary(), toRelease);
}

6. Token Approval Check in terminateVesting()

If the contract does not hold enough token approvals, the safeTransfer() calls could revert unexpectedly, which might not be obvious.

Mitigation:
You could add a pre-transfer balance check to gracefully handle insufficient token balances, although this is optional if you're certain approvals will always be sufficient.


Summary of Recommended Changes:

  1. Fix Reentrancy Risk: Move vestingTerminated = true before token transfers.

  2. Add Zero Balance Checks: Prevent unnecessary transfers.

  3. Consider onlyBeneficiary Modifier: Restrict access to releaseRemaining().

  4. Check Token Approval: Optional, for better handling of transfer reverts.

  5. Be Aware of Timestamp Manipulation: Consider using block.number if precise timing is required.

These changes will help make your contract more secure, gas-efficient, and user-friendly.

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.