The stake() function allows users to stake tokens on behalf of another address by passing the account argument. However, unstaking via requestUnstake(), cancelUnstakeRequests(), or unstake() requires the caller (msg.sender) to match the staked address, as these functions do not accept an account argument. This inconsistency could result in funds becoming permanently stuck if tokens are staked to an address without a private key or control.

See their function signatures:

function stake(address account, uint256 amount) external;
function requestUnstake(uint256 requestId, uint256 amount) external;
function cancelUnstakeRequests(uint256[] memory requestIds) external;
function unstake(uint256[] memory requestIds) external;

If tokens are staked to an inaccessible or unintended address, there is no mechanism to recover these funds, leading to a potential loss.

This issue can be mitigated through one or more of the following solutions:

• Remove the account argument from the stake() function to ensure users can only stake tokens for themselves

• Implement a delegated unstaking mechanism, allowing users to authorize trusted accounts to unstake on their behalf.

• Document the risk of staking to unintended addresses, ensuring users understand the potential for permanent fund loss in such scenarios.

By addressing this inconsistency, the contract can better align user expectations with its functionality and reduce the risk of lost funds.

PARTIALLY SOLVED: The BSX team partially fixed this finding in commit 4e2e968 by introducing a new stake() function without the account argument to minimize the risk of users mistakenly staking to an unintended address. This provides a safer option for regular staking operations. However, the original stake() function with the account argument remains accessible, leaving the potential for incorrect usage. To fully address the risk, it is recommended to document the intended use of the original stake() function and clearly warn users of the potential consequences of staking to an unintended or inaccessible address.

During the security assessment, several functions in the smart contracts were found to lack adequate input validation. Without proper checks, critical parameters may be set to invalid, undesired, or unrealistic values, which could lead to vulnerabilities, unexpected behavior, or erroneous contract states.

For instance, the initialize() function does not validate its inputs. Examples of recommended validation include:

require(_owner != address(0), "Owner cannot be zero address");
require(address(_stakingToken) != address(0), "Staking token cannot be zero address");
require(_rewardRate > 0, "Reward rate must be greater than zero");
require(_rewardPeriod > 0, "Reward period must be greater than zero");

Failure to validate inputs can result in critical issues:

• If _rewardPeriod is set to 0, the getAccumulatedRewardPerToken() function would attempt a division by zero, leading to contract failures.

• Assigning zero addresses to _owner or _stakingToken would result in misconfigured or unusable contract states.

While initialize() is a clear example, other functions across the codebase may similarly lack proper input validation. A comprehensive review should be conducted to identify and address these gaps.

To mitigate these issues, implement robust input validation across all critical functions, including constructors and initialization functions. For example:

• Ensure parameters meet logical constraints (e.g., non-zero addresses, positive values).

• Prevent division-by-zero and other edge cases through early checks.

A thorough review of the codebase is recommended to identify all functions requiring input validation. Establishing consistent validation standards enhances contract security, reliability, and predictability, while reducing the risk of vulnerabilities caused by invalid inputs.

SOLVED: The BSX team fixed this finding in commit 9f6d709 by introducing more validations to the user inputs.

The StakingVault contract contains an instance of an ERC20 operation that does not utilize OpenZeppelin's SafeERC20 library, which is considered best practice for securely handling token transfers. Specifically, the stake() function uses the raw transferFrom() function instead of the safer safeTransferFrom() provided by SafeERC20. While the unstake() function correctly uses safeTransfer(), this inconsistency introduces potential risks if the stakingToken does not adhere strictly to the ERC20 standard (e.g., tokens that do not revert on failed transfers).

Instance in the stake() function:

IERC20(stakingToken).transferFrom(msg.sender, address(this), amount);

The risk of this finding is reduced from low to informational because:

• The stakingToken is intended to be the BSX token, which conforms strictly to the ERC20 standard and returns proper boolean values for transferFrom.

• The project does not support other tokens as the staking token, reducing the likelihood of compatibility issues.

Update the stake() function to use OpenZeppelin's safeTransferFrom() for consistency and to ensure secure token handling.

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

It was identified that the StakingVault contract inherited from OpenZeppelin's Ownable2StepUpgradeable library. In the Ownable2StepUpgradeable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

The following Foundry test was used in order to prove the aforementioned issue:

function test_renounceOwnership() public {
    vm.assertEq(stakingVault.owner(), owner);
    vm.assertEq(stakingVault.pendingOwner(), address(0));

    vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(this)));
    stakingVault.renounceOwnership();
    vm.assertEq(stakingVault.owner(), owner);
    vm.assertEq(stakingVault.pendingOwner(), address(0));

    vm.prank(owner);
    stakingVault.renounceOwnership();
    vm.assertEq(stakingVault.owner(), address(0));
    vm.assertEq(stakingVault.pendingOwner(), address(0));
}

To run it, just execute the following forge command:

forge test --mt test_renounceOwnership -vvvv

Observe that the test passes and the ownership has been renounced (address(0)).

Ran 1 test for test/StakingVault.t.sol:StakingVaultTest
[PASS] test_renounceOwnership() (gas: 33390)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.14ms (75.00µs CPU time)

Ran 1 test suite in 156.22ms (1.14ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

ACKNOWLEDGED: The BSX team indicated that they acknowledge the risk of this security finding, indicating that renouncing ownership in the future is a deliberate and desired aspect of the contract's functionality.

When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this results in an extra sload operation (100 additional gas for each iteration except the first). For memory arrays, this results in an extra mload operation (3 additional gas for each iteration except the first).

Detecting loops that use the length member of a storage array in their loop condition without modifying it can reveal opportunities for optimization. See the following example in StakingVault.sol:

function cancelUnstakeRequests(uint256[] memory requestIds) external nonReentrant updateReward(msg.sender) {
  if (requestIds.length == 0) revert EmptyRequestIds();

  address account = msg.sender;
  uint256 amount = 0;
  for (uint256 i = 0; i < requestIds.length; i++) {
    ...
}

Cache the length of the array in a local variable outside the loop to optimize gas usage. This reduces the number of read operations required during each iteration of the loop. See the example fixes below:

function cancelUnstakeRequests(uint256[] memory requestIds) external nonReentrant updateReward(msg.sender) {
  if (requestIds.length == 0) revert EmptyRequestIds();

  address account = msg.sender;
  uint256 amount = 0;
  uint256 length = requestIds.length;
  for (uint256 i = 0; i < length; i++) {
    ...
}

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

The files in scope currently use floating pragma version ^0.8.25, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.25, and less than 0.9.0. It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version to the same version used during development and testing.

ACKNOWLEDGED: The BSX team indicated that they acknowledge the risk of this security finding.

The project accepts using a Solidity compiler version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

For example, on StakingVault.sol, instead of declaring:

mapping(address => uint256) public lastAccumulatedRewards;

It could be declared as:

mapping(address user => uint256 amount) public lastAccumulatedRewards;

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

During the security assessment of the smart contracts, an instance of unused import was found. Unused imports can clutter the codebase, reducing readability and potentially leading to confusion during development or auditing. Additionally, unnecessary imports can slightly increase the compiled contract's bytecode size, potentially affecting deployment and execution costs.

In src/StakingVault.sol:

import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";

For better clarity, consider removing all unused code.

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

The functions cancelUnstakeRequests() and unstake() accept requestIds as a memory array, even though the array is not mutated in the external function itself. This results in unnecessary gas overhead when copying data from calldata to memory during the abi.decode() process.

Using calldata directly for such arguments bypasses the copying loop, reducing gas costs, especially for larger arrays.

Optimizing cancelUnstakeRequests() and unstake() to accept the requestIds array as calldata instead of memory can reduce gas costs for external calls. The savings grow with the size of the input array.

Consider updating the function signatures as follows:

function cancelUnstakeRequests(uint256[] calldata requestIds) external nonReentrant updateReward(msg.sender) {
    // Function logic remains the same
}

function unstake(uint256[] calldata requestIds) public nonReentrant {
    // Function logic remains the same
}

By switching the argument type to calldata, the requestIds array is read directly from the transaction's calldata, eliminating unnecessary memory allocations and reducing gas costs.

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

In Solidity development, adhering to the official style guide is best practice to ensure code consistency, readability, and maintainability. Throughout the contracts, there are several instances where the code does not follow these guidelines. Some examples include:

• In BsxToken, the NAME, SYMBOL and TOTAL_SUPPLY internal constants do not start with an underscore.

• The unstake() function is declared as public, but is never used inside the contract.

• In BsxToken, the TOTAL_SUPPLY constant does not use scientific notation.

• Redundant IERC20 casting in stake() for calling the transferFrom() function.

Apply the following style guide improvements throughout the codebase:

• In BsxToken, the declared internal constants should start with an underscore: _NAME, _SYMBOL and _TOTAL_SUPPLY.

  
  

• In StakingVault.sol, the unstake() function could be declared as external.

  
  

• For TOTAL_SUPPLY constant could use scientific notation (1e9).

  
  

• In StakingVault.sol, remove the redundant IERC20 casting in the stake() function (also switched from transferFrom to safeTransferFrom as indicated in the finding Unsafe ERC20 Operations).

SOLVED: The BSX team fixed this finding in commit 0c8fb29 by implementing the recommended solution.

The BsxToken contract includes redundant inheritance, which unnecessarily increases the complexity of the contract. Specifically, it inherits from ERC20, which is already inherited by ERC20Permit and ERC20Votes:

contract BsxToken is ERC20, ERC20Permit, ERC20Votes {

This creates a redundancy that can complicate contract structure and increase the risk of potential issues related to Solidity's C3 linearization for resolving multiple inheritance.

To simplify the contract and eliminate potential linearization issues, consider removing the redundant ERC20 inheritance from BsxToken:

contract BsxToken is ERC20Permit, ERC20Votes {

ACKNOWLEDGED: The BSX team indicated that they acknowledge the risk of this security finding indicating that an explicit design was intentionally preferred.