During the security review, it was discovered that, even if a user with the pauser role successfully called pauseClaim() to pause the claiming functionalities, it is still possible to call the claim() and claimBatch() functions. This indicates that the pausing mechanism for claim functions is ineffective, potentially allowing unauthorized claims during the paused state.

This is because none of the two claiming functionalities, claim() and claimBatch(), had the whenNotPausedToken(PAUSE_TOKEN_CLAIM, _token) modifier:

function claim(address _user, uint256 _orderId) external nonReentrant returns (uint256) {

function claimBatch(address _user, uint256[] calldata _orderIds) external nonReentrant returns (uint256) {

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

function testStakeManagerClaimPaused() public {
    vm.startPrank(admin);
    accessManager.grantGovernanceRole(governance);
    accessManager.grantStakeManagerRole(address(stakeManager));
    vm.stopPrank();

    vm.startPrank(governance);
    stakeManager.addTokenToWhitelist(address(ethwBTC));
    stakeManager.setDepositCap(address(wBTCToken), 150 ether);
    vm.stopPrank();

    // Getting some tokens to test
    wBTCToken.mint(address(this), 1_000 ether);
    wBTCToken.approve(address(stakeManager), 500 ether);

    // Deposit & withdraw
    stakeManager.deposit(address(wBTCToken), 100 ether);
    stakeManager.withdraw(address(wBTCToken), 100 ether);

    // Grant permissions and pause token claim
    vm.startPrank(admin);
    assertTrue(accessManager.grantPauserRole(admin));
    pauseManager.pauseClaim((address(wBTCToken)));
    assertTrue(pauseManager.isPausedToken(PAUSE_TOKEN_CLAIM, address(wBTCToken)));
    vm.stopPrank();

    skip(block.timestamp + 5 minutes + 1);
    vm.expectRevert(ActionPaused.selector);
    stakeManager.claim(address(this), 1);
}

To run it, just execute the following forge command:

forge test --mt testStakeManagerClaimPaused --force -vvvv

Observe that the test fails because it does not revert as expected.

[FAIL. Reason: call did not revert as expected] testStakeManagerClaimPaused() (gas: 695149)
...
    ├─ [0] VM::expectRevert(ActionPaused())
    │   └─ ← [Return] 
    ├─ [36667] ERC1967Proxy::claim(StakeManagerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1)
    │   ├─ [36271] StakeManager::claim(StakeManagerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1) [delegatecall]
    │   │   ├─ [28462] ERC1967Proxy::processClaim(1)
    │   │   │   ├─ [28069] EthwBTC::processClaim(1) [delegatecall]
    │   │   │   │   ├─ [734] AccessManager::isStakeManager(ERC1967Proxy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1]) [staticcall]
    │   │   │   │   │   └─ ← [Return] true
    │   │   │   │   ├─ [3310] MockToken::transfer(StakeManagerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 100000000000000000000 [1e20])
    │   │   │   │   │   ├─ emit Transfer(from: ERC1967Proxy: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], to: StakeManagerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], value: 100000000000000000000 [1e20])
    │   │   │   │   │   └─ ← [Return] true
    │   │   │   │   └─ ← [Return] 100000000000000000000 [1e20]
    │   │   │   └─ ← [Return] 100000000000000000000 [1e20]
    │   │   ├─ emit LogClaim(_token: MockToken: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], _user: StakeManagerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], _orderId: 1, amountToken: 100000000000000000000 [1e20])
    │   │   └─ ← [Return] 100000000000000000000 [1e20]
    │   └─ ← [Return] 100000000000000000000 [1e20]
    └─ ← [Revert] call did not revert as expected

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.21s (306.54µs CPU time)q

Ensure that the claim() and claimBatch() functions check if the claim is paused before executing. This can be done by adding a modifier or an inline check to verify the paused state. A proposed solution would be:

function claim(address _user, uint256 _orderId) external nonReentrant whenNotPausedToken(PAUSE_TOKEN_CLAIM, _token) returns (uint256) {

function claimBatch(address _user, uint256[] calldata _orderIds) external nonReentrant whenNotPausedToken(PAUSE_TOKEN_CLAIM, _token) returns (uint256) {

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by adding the whenNotPausedToken modifier to the claim() and claimBatch() functions.

The codebase utilizes several outdated or non-official third-party libraries, which could pose security and maintenance risks. Specifically:

1. The library LowGasSafeMath is being used, although it is redundant with Solidity 0.8.0 and above, which natively handles overflows and underflows.

2. The library TransferHelper is in use, but it lacks some advanced functionalities provided by OpenZeppelin's SafeERC20, such as safeIncreaseAllowance() and safeDecreaseAllowance().

3. For testing and deployment, an unofficial third-party dependency foundry-upgrades (see: foundry-upgrades) is used. It is recommended to switch to the official OpenZeppelin counterpart (see: openzeppelin-foundry-upgrades) to ensure better support and security.

1. Remove Redundant Libraries:

   Replace the LowGasSafeMath library with native Solidity arithmetic operations to leverage built-in overflow and underflow checks provided by Solidity 0.8.0 and above. This change will reduce unnecessary dependencies and simplify the codebase.

   
   

2. Adopt Industry-Standard Libraries:

   Replace the TransferHelper library with OpenZeppelin's SafeERC20 library. The SafeERC20 library offers additional functionalities such as safeIncreaseAllowance() and safeDecreaseAllowance(), providing more comprehensive and secure token handling.

   
   

3. Switch to Official Upgrades Library:

   Replace the unofficial foundry-upgrades dependency with the official openzeppelin-foundry-upgrades library from OpenZeppelin. The official library is likely to be more secure, well-maintained, and supported by the community. Remove the foundry-upgrades dependency if it is no longer needed.

By implementing these recommendations, the codebase will be more secure, maintainable, and aligned with industry standards, reducing potential risks associated with using outdated or unofficial third-party libraries.

Remediation Plan

SOLVED: The OrangeLayer team removed the use of the LowGasSafeMath and TransferHelper libraries (although they are still present in the project) and switched the use of the unofficial foundry-upgrades library to the one from OpenZeppelin (openzeppelin-foundry-upgrades library). For the sake of simplicity, it would be recommended to delete the LowGasSafeMath and TransferHelper libraries altogether.

Most contracts use the delegateCall proxy pattern, and hence their implementations require the use of initialize() functions instead of constructors. This requires derived contracts to call the corresponding init functions of their inherited base contracts. This is done in most places except in the EthwBTC contract, which is inheriting from ReentrancyGuardUpgradeable, but its initialize() function is missing the call to __ReentrancyGuard_init(). Observe the initialize() function in use:

/**
 * @dev Initializes the contract with required parameters.
 * @param _token Address of the underlying token.
 * @param _accessManager Address of the AccessManager contract.
 * @param _pauseManager Address of the PauseManager contract.
 */
function initialize(address _token, address _accessManager, address _pauseManager) external initializer onlyProxy {
    if (_token == address(0) || _accessManager == address(0) || _pauseManager == address(0))
        revert InputAddressZero();

    __UUPSUpgradeable_init();

    underlyingToken = _token;
    accessManager = IAccessManager(_accessManager);
    pauseManager = IPauseManager(_pauseManager);
}

Add the missing __ReentrancyGuard_init() call to the initialize() function of the EthwBTC contract as suggested below:

/**
 * @dev Initializes the contract with required parameters.
 * @param _token Address of the underlying token.
 * @param _accessManager Address of the AccessManager contract.
 * @param _pauseManager Address of the PauseManager contract.
 */
function initialize(address _token, address _accessManager, address _pauseManager) external initializer onlyProxy {
    if (_token == address(0) || _accessManager == address(0) || _pauseManager == address(0))
        revert InputAddressZero();

    __UUPSUpgradeable_init();
    __ReentrancyGuard_init();

    underlyingToken = _token;
    accessManager = IAccessManager(_accessManager);
    pauseManager = IPauseManager(_pauseManager);
}

Remediation Plan

SOLVED: The OrangeLayer team added the missing __ReentrancyGuard_init() call to the initialize() function of the EthwBTC contract as suggested.

At least all public and external functions that are not view or pure should have NatSpec comments. During the security review, it was observed that the multiple publicly-accessible functions did not have any NatSpec comments.

Moreover, the test suite's coverage is insufficient, representing a less-than-ideal practice and failing to ensure thorough validation of function behaviors and overall contract functionality.

Finally, the project could not be compiled directly from the GitHub repository due to compilation errors. These errors had to be resolved before the security review could begin, indicating potential issues with the project's setup. See the following screenshots for more details on the compilation errors:

Consider adding NatSpec documentation to all functions, specially those that are publicly-accessible.

Additionally, expanding the test suite's coverage is essential to effectively validate function functionalities and ensure thorough contract testing. Enhancing the test suite represents a best practice, instilling confidence in contract behavior, mitigating the risk of undetected bugs, and streamlining the development process. By addressing these areas, the project can foster transparency, reliability, and robustness, thereby enhancing its overall quality and user experience.

Finally, resolve any compilation errors present in the project to ensure that it can be compiled successfully from the GitHub repository.

Remediation Plan

PENDING: The OrangeLayer team indicated that they are currently working on test cases and improving their NatSpec comments.

The codebase contains several instances of ToDo comments and commented-out code. While these do not directly cause security vulnerabilities or impact the audit outcome as long as the code is functional, their presence can aid attackers in identifying potential attack surfaces. Additionally, the existence of development comments suggests that the audited code may differ from the final released version, increasing the risk of new vulnerabilities being introduced post-audit.

Example of a ToDo comment suggesting a further update of the code:

/**
 * @dev Upgrades the contract to a new implementation.
 * @param _newImplementation The address of the new implementation contract.
 #TODO Pausability check before upgrade
    */
function upgradeTo(address _newImplementation) external onlyProxy onlyUpgrader {
    _authorizeUpgrade(_newImplementation);
    upgradeToAndCall(_newImplementation, "");
}

Ensure that all ToDo comments and commented-out lines are removed from the codebase before deployment. These elements can provide insights into unfinished or potentially insecure parts of the code to an attacker.

Remediation Plan

PARTIALLY SOLVED: During the remediation review, it was found that new lines of code were commented out and there are still some ToDo comments.

The setWithdrawalDelay() function of the StakeManager contract contains redundant input validation checks that add unnecessary complexity and can increase gas costs. Solidity inherently ensures that the _delay parameter is a valid member of the WithdrawalDelay enum, making these additional checks redundant.

/// @notice Withdrawal delay options.
enum WithdrawalDelay {
  NoDelay,
  BasicDelay,
  DelegationDelay
}

...

function setWithdrawalDelay(WithdrawalDelay _delay) external onlyGovernance whenNotPaused(PAUSE_LISTING_UPDATE) returns (bool) {
  // Input validation
  if (
    _delay != WithdrawalDelay.NoDelay &&
    _delay != WithdrawalDelay.BasicDelay &&
    _delay != WithdrawalDelay.DelegationDelay
  ) revert InvalidWithdrawalDelay();
...

It is recommended to eliminate the unnecessary validation checks within the setWithdrawalDelay() function. Solidity guarantees that the _delay parameter is a valid member of the WithdrawalDelay enum, so these checks do not add any additional safety.

Remediation Plan

PARTIALLY SOLVED: While the redundant input validation was indeed removed, during the remediation review it was found that the new validation is way less restrictive now. The _delay parameter was changed from being of type WithdrawalDelay enum to uint. This change means that both validations have been removed (the implicit for using the enum and the explicit that was highlighted as redundant). Now, the affected function has a way more lax validation:

1. The _delay parameter can contain an arbitrary number.

2. The condition

if (_delay <= 0) revert InvalidWithdrawalDelay();

could be changed to:

if (_delay == 0) revert InvalidWithdrawalDelay();

as _delay cannot be less than 0 as it is a uint number.

It is recommended to evaluate whether the new permissive withdrawalDelayBlocks allowed values make sense.

The EthwBTC contract inherits from ReentrancyGuardUpgradeable to protect against reentrancy attacks. However, it does not use the nonReentrant modifier in any of its functions. This oversight can leave the contract vulnerable to reentrancy attacks, where an attacker repeatedly calls a function before its initial execution is complete, potentially causing unexpected behavior and financial loss.

Review all external and public functions in the EthwBTC contract and apply the nonReentrant modifier to functions that involve state changes, external calls, or transfers of Ether or tokens. This ensures protection against reentrancy attacks. If the nonReentrant modifier is not needed, consider removing the ReentrancyGuardUpgradeable inheritance.

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by adding the nonReentrant modifier to protect functions interacting with other contracts.

The abstract contract CoStake is meant to be inherited by other contracts. However, it does not contain a __gap variable, even though it is upgradeable. This means that in the event of adding new state variables when upgrading the contract, it can lead to storage slot collisions.

Notice that, by inspecting the storage slots of the CoStake contract, the slot 3 is the last one (used by the global state variable withdarwOrderInfo):

forge inspect src/CoStake.sol:CoStake storage --pretty
| Name              | Type                                                  | Slot | Offset | Bytes | Contract                |
|-------------------|-------------------------------------------------------|------|--------|-------|-------------------------|
| underlyingToken   | address                                               | 0    | 0      | 20    | src/CoStake.sol:CoStake |
| totalShares       | uint256                                               | 1    | 0      | 32    | src/CoStake.sol:CoStake |
| userInfos         | mapping(address => struct ICoStake.UserInfo)          | 2    | 0      | 32    | src/CoStake.sol:CoStake |
| withdarwOrderInfo | mapping(uint256 => struct ICoStake.WithdarwOrderInfo) | 3    | 0      | 32    | src/CoStake.sol:CoStake |

Notice that EthwBTC also has slot 3 used by the global state variable withdarwOrderInfo, and then its own global state variables:

forge inspect src/networks/EthereumBTC.sol:EthwBTC storage --pretty
| Name              | Type                                                  | Slot | Offset | Bytes | Contract                         |
|-------------------|-------------------------------------------------------|------|--------|-------|----------------------------------|
| underlyingToken   | address                                               | 0    | 0      | 20    | networks/EthereumBTC.sol:EthwBTC |
| totalShares       | uint256                                               | 1    | 0      | 32    | networks/EthereumBTC.sol:EthwBTC |
| userInfos         | mapping(address => struct ICoStake.UserInfo)          | 2    | 0      | 32    | networks/EthereumBTC.sol:EthwBTC |
| withdarwOrderInfo | mapping(uint256 => struct ICoStake.WithdarwOrderInfo) | 3    | 0      | 32    | networks/EthereumBTC.sol:EthwBTC |
| accessManager     | contract IAccessManager                               | 4    | 0      | 20    | networks/EthereumBTC.sol:EthwBTC |
| pauseManager      | contract IPauseManager                                | 5    | 0      | 20    | networks/EthereumBTC.sol:EthwBTC |

In the event of adding a new global state variable to CoStake, it could enter into conflict with EthwBTC.

For more information, see: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

It is recommended to declare a fixed-size array in the base contract with an initial number of slots. This can be an array of uint256 so that each element reserves a 32 byte slot. Use the name __gap or a name starting with __gap_ for the array so that OpenZeppelin Upgrades will recognize the gap:

abstract contract CoStake is ICoStake {
  ...
  uint256[46] __gap;
}

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by adding a __gap global state variable at the end of the CoStake abstract contract.

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.

// Find the index of the token in the array
uint256 indexToRemove;
for (uint256 i = 0; i < tokenList.length; i++) {
    if (tokenList[i] == _token) {
        indexToRemove = i;
        break;
    }
}

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.

// Find the index of the token in the array
uint256 indexToRemove;
uint256 tokenListLength = tokenList.length;
for (uint256 i = 0; i < tokenListLength; i++) {
    if (tokenList[i] == _token) {
        indexToRemove = i;
        break;
    }
}

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by updating how the removeTokenFromWhitelist() works. During the remediation review it was found that the affected for loop is now commented out. While this finding has been fixed, consider removing all commented out code as indicated in the finding ToDo Comments and Commented Out Code.

The whenNotPaused modifier and the related _whenNotPaused() function of the EthwBTC contract are not utilized by any of the functions. Additionally, the _whenNotPaused() function is empty, as its body is commented out. This indicates that the contract is not leveraging the intended pause functionality, potentially leaving it without the ability to restrict actions during paused states.

/**
 * @dev Modifier to check if the contract is not paused by a specific pauser. 
 * @param _pauser The address of the pauser.
 */
modifier whenNotPaused(bytes32 _pauser) {
  _whenNotPaused(_pauser);
  _;
}

...

/**
 * @dev Checks if the contract is not paused by a specific pauser.
 * @param _pauser The address of the pauser.
 */
function _whenNotPaused(bytes32 _pauser) internal view {
    // if (!pauseManager.isPaused(_pauser)) revert ActionPaused();
}

If the pause functionality is not required, consider removing the whenNotPaused modifier and the _whenNotPaused() function from the contract to reduce unnecessary complexity and maintain a clean codebase. Otherwise, uncomment and properly implement the _whenNotPaused() function to check the paused state. This will enable the whenNotPaused modifier to effectively restrict function calls.

Finally, ensure that the pause functionality is well-documented, explaining when and how the contract can be paused and unpaused, and the implications for the contract's operations. This helps developers and auditors understand the intended behavior and ensures proper usage.

By following these recommendations, the contract will either effectively utilize the pause functionality to enhance security and control or maintain a clean codebase by removing unnecessary components.

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by making use of the whenNotPaused modifier.

In the StakeManager contract, there is a global state variable declared named claimDelay. However, upon review, it appears that this variable is never referenced or utilized within the contract's functions or logic.

// Claim delay
uint256 internal claimDelay;

Unused variables should be cleaned up from the code if they have no purpose. Clearing these variables can reduce gas usage during the deployment of contracts, and also increases the readability of the code.

More specifically, consider removing the unused claimDelay variable to declutter the contract codebase and improve readability. Alternatively, if claimDelay serves a purpose that is not immediately evident, ensure that it is properly integrated into the contract's functionality to avoid unnecessary complexity and maintain code efficiency.

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by commenting the unused state variable out. While this finding has been fixed, consider removing all commented out code as indicated in the finding ToDo Comments and Commented Out Code.

In CoStake.sol, there are instances where functions declare named return variables but do not use them, resulting in redundant code. Specifically:

The function _getUserDetails declares UserInfo memory userInfo but returns userInfos[_user] instead of using the named return variable.

function _getUserDetails(address _user) internal view returns (UserInfo memory userInfo){
  return userInfos[_user];
}

The function _getWithdrawOrderInfo declares WithdrawOrderInfo memory woi but returns withdrawOrderInfo[_orderId] instead of using the named return variable.

function _getWithdrawOrderInfo(uint256 _orderId) internal view returns (WithdarwOrderInfo memory woi){
  return withdarwOrderInfo[_orderId];
}

These declarations are unnecessary and can be simplified.

Finally, in the related functions of the StakeManager contract, named returns are not used, even though they could to simplify things further:

function getUserDetails(address _token) external view returns (ICoStake.UserInfo memory){
    // Retrieve user details from the staking contract
    ICoStake.UserInfo memory userinfo;
    userinfo = ICoStake(tokensMeta[_token].stakerCore).getUserDetails(msg.sender);
    return userinfo;
}

and:

function getOrderIdDetails(address _token, uint256 _orderId) public view returns (ICoStake.WithdarwOrderInfo memory) {
    // Retrieve withdrawal order details from the staking contract
    ICoStake.WithdarwOrderInfo memory withdarwOrderInfo;
    withdarwOrderInfo = ICoStake(tokensMeta[_token].stakerCore).getOrderIdDetails(_orderId);
    return withdarwOrderInfo;
}

Consider removing the unused named return variables and just specifying the return type in the function signature.

function _getUserDetails(address _user) internal view returns (UserInfo memory) {
  return userInfos[_user];
}

function _getWithdrawOrderInfo(uint256 _orderId) internal view returns (WithdarwOrderInfo memory) {
  return withdarwOrderInfo[_orderId];
}

Finally, notice the related function on StakeManager could be simplified too by doing:

function getUserDetails(address _token) external view returns (ICoStake.UserInfo memory userinfo) {
    // Retrieve user details from the staking contract
    userinfo = ICoStake(tokensMeta[_token].stakerCore).getUserDetails(msg.sender);
}

and

function getOrderIdDetails(address _token, uint256 _orderId) public view returns (ICoStake.WithdarwOrderInfo memory withdarwOrderInfo) {
    // Retrieve withdrawal order details from the staking contract
    withdarwOrderInfo = ICoStake(tokensMeta[_token].stakerCore).getOrderIdDetails(_orderId);
}

Remediation Plan

ACKNOWLEDGED: The OrangeLayer team acknowledged this finding by leaving the mentioned functions intact.

The project is using Solidity 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 StakeManager, instead of declaring:

mapping(address => uint256[]) public userOrdersList;

It could be declared as:

mapping(address user => uint256[] orderList) public userOrdersList;

Remediation Plan

SOLVED: The OrangeLayer team solved the issue by using named mappings as suggested.

The following comment was found in the CoStake contract:

// TODO snaity check to make sure token are transferred

The word snaity above should be spelled as sanity instead.

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

Remediation Plan

ACKNOWLEDGED: This finding has been marked as acknowledged because there are still some typographical errors in the code.

The EthereumBTC.sol file currently uses floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.20, 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.

Consider locking the pragma version to the same version used during development and testing.

Remediation Plan

ACKNOWLEDGED: The OrangeLayer team acknowledged this finding by not fixing it leaving pragma version as >=0.8.20 <0.9.0. This would allow the contracts to be compiled by any compiler version that is greater than or equal to 0.8.20, and less than 0.9.0.