StakingRewardsProxy Manual Review Findings

SRP-01M: Inexistent Capability of Native Asset Extraction

Type	Severity	Location
Logical Fault		StakingRewardsProxy.sol:L142-L144

Description:

The emergency system of the contract is permanent, however, no function exists to extract native assets sent to the contract in such a scenario that were originally meant to be used for relaying cross-chain calls.

Impact:

If the system is set to emergency mode, any native assets held within it will be permanently locked in the contract.

Example:

Copy
142function emergency() external onlyOwner {
143    paused = 1;
144}
145
146function emergencyWithdraw() external {
147    require(paused == 1, "StakingRewardsProxy: contract is not paused");
148    require(balances[msg.sender] > 0, "StakingRewardsProxy: Invalid withdrawal, no deposits done");
149
150    stakingToken.transfer(msg.sender, balances[msg.sender]);
151    emit Withdrawn(msg.sender, balances[msg.sender]);
152
153    balances[msg.sender] = 0;
154}
155
156function clearQueue(address _user) external onlyOwner {
157    delete actionInQueue[_user];
158    delete signatures[_user];
159}
160
161receive() external payable {}

Recommendation:

We advise a function to be coded that permits the native asset to be extracted to the owner should the system be in emergency mode ensuring no funds can be permanently lost in the contract.

Alleviation:

A native asset transfer was introduced in the emergency function of the system that forwards the native asset balance to its caller. We would like to note that this can become unsafe as the transfer call forwards a fixed gas stipend that can break; we advise the usage of a library such as OpenZeppelin's Address that exposes the sendValue function performing a native asset transfer and forwarding arbitrary gas along with it to guarantee it succeeds its execution.

SRP-02M: Inexistent Conformance to Checks-Effects-Interactions Pattern

Type	Severity	Location
Reentrant Call		StakingRewardsProxy.sol:L150, L153

Description:

The linked code does not conform to the Checks-Effects-Interactions (CEI) pattern and zeroes out the balance distributed to the user after the transfer is performed.

Impact:

If the stakingToken conforms to one of the re-entrant token standards such as EIP-777, a re-entrancy attack can unfold here whereby the contract is siphoned of all the stakingToken funds it possesses.

Example:

Copy
146function emergencyWithdraw() external {
147    require(paused == 1, "StakingRewardsProxy: contract is not paused");
148    require(balances[msg.sender] > 0, "StakingRewardsProxy: Invalid withdrawal, no deposits done");
149
150    stakingToken.transfer(msg.sender, balances[msg.sender]);
151    emit Withdrawn(msg.sender, balances[msg.sender]);
152
153    balances[msg.sender] = 0;
154}

Recommendation:

We advise the balance to be zeroed out prior to the outward token transfer to ensure the code is conformant to the CEI pattern and prevents re-entrancy attacks from manifesting.

Alleviation:

The statements have been properly re-ordered to nullify the balance entry and then transfer the amount of funds that the balance was holding thus conforming to the CEI pattern and alleviating this exhibit.

SRP-03M: Authoritative Overriddence of Cross-Chain Data Synchrosity

Type	Severity	Location
Logical Fault		StakingRewardsProxy.sol:L156-L159

Description:

The clearQueue function permits the owner of the contract to override the pending action and signature of a particular account, an action that can de-stabilize the accounting of the multi-chain staking system.

Impact:

An improper clearance instruction can cause different types of messages to be relayed in an out-of-order fashion given that all contracts implement non-blocking LayerZero receiver implementations, ultimately leading to fund loss due to improper accounting.

Example:

Copy
156function clearQueue(address _user) external onlyOwner {
157    delete actionInQueue[_user];
158    delete signatures[_user];
159}

Recommendation:

We advise the function to be removed from the codebase as it can significantly impact the project's operational integrity if misused.

Alleviation:

The clearQueue has been removed from the codebase as advised thus alleviating this exhibit.

SRP-04M: Hard-Coded Gas Allowance for Cross-Chain Relay

Type	Severity	Location
Language Specific		StakingRewardsProxy.sol:L65

Description:

The cross-chain relay transaction created by _sendMessage forwards a fixed gas stipend that can cause the target chain to run into out-of-gas errors.

Impact:

As the EVM gas costs are dynamic, out-of-gas errors can frequently occur when a fixed gas stipend is provided and thus rendering the contract cross-chain inoperable.

Example:

Copy
63// use adapterParams v1 to specify more gas for the destination
64uint16 version = 2;
65uint256 gasForDestinationLzReceive = 350000;
66uint256 nativeForDst = 300000000; // hardcoded, to be improved in the future
67bytes memory adapterParams = abi.encodePacked(version, gasForDestinationLzReceive, nativeForDst, controller);

Recommendation:

We advise a configurable list of gas allocations for each _action to be implemented in the contract to allow more granular control over how much gas is allocated for each call as well as ensure that future changes to the EVM will not cause the contract to be inoperable due to insufficient gas.

Alleviation:

A dedicated gas related entry has been introduced to the contract that allows the gas allocation for each of the five actions supported by the proxy (withdrawals, claims, and three controller related actions) to be configurable to an arbitrary level thereby alleviating this exhibit in full.

SRP-05M: Improper Invocations of EIP-20 transfer

Type	Severity	Location
Standard Conformity		StakingRewardsProxy.sol:L126, L150

Description:

The linked statement do not properly validate the returned bool of the EIP-20 standard transfer function. As the standard dictates, callers must not assume that false is never returned.

Impact:

If the code mandates that the returned bool is true, this will cause incompatibility with tokens such as USDT / Tether as no such bool is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.

Example:

Copy
126stakingToken.transfer(target, withdrawAmount);

Recommendation:

Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists in each instance.

Alleviation:

Both referenced transfer invocations have been properly replaced by their safe-prefixed counterparts as advised.

SRP-06M: Potentially Incorrect Chain ID

Type	Severity	Location
Language Specific		StakingRewardsProxy.sol:L21

Description:

The StakingRewardsController implementation is meant to be deployed on the Polygon blockchain based on the documentation of the project, however, the chain ID utilized by the contract as a controllerChainId points to 10012 which does not match the Polygon mainnet nor the Polygon testnet.

Impact:

An incorrectly configured chain ID for the relayer of LayerZero can cause significant issues in cross-chain message delivery.

Example:

Copy
21uint16 public immutable controllerChainId = 10012;

Recommendation:

We advise the chain ID to be corrected or documented as to what it is meant to point to as it currently appears invalid.

Alleviation:

The original controller chain ID was identifying the Fantom testnet ID and now the code has been updated to point to the Fantom mainnet ID (12) as that is the intended deployment environment of the Dapp Radar solution. As a result of the aforementioned, we consider this exhibit nullified given that the original implementation was proper.

SRP-07M: Incorrect Withdrawal Relay System

Type	Severity	Location
Logical Fault		StakingRewardsProxy.sol:L95, L97, L122, L123

Description:

The current staking system utilizes a flawed withdrawal relay mechanism whereby a withdrawal operation is requested and withdraws the full amount of the user as calculated by the controller rather than the proxy. This permits a vulnerability whereby a withdrawal is requested, a stake operation is performed after it and the withdrawal operation is relayed back. This will cause all newly staked funds to be lost as the "process" operation of the withdrawal zeroes out the balances entry instead of comparing it with the withdrawAmount.

Impact:

The current withdrawal system can lead to loss of funds depending on the order of operations submitted by the user.

Example:

Copy
90function withdraw(bytes memory _signature) external payable notPaused notInQueue(msg.sender) {
91    require(balances[msg.sender] > 0, "StakingRewardsProxy: Nothing to withdraw");
92
93    actionInQueue[msg.sender] = ACTION_WITHDRAW;
94    signatures[msg.sender] = _signature;
95    uint256 amount = 0;
96    emit WithdrawalInitiated(msg.sender);
97    _sendMessage(ACTION_WITHDRAW, amount, _signature);
98}
99
100function claim(bytes memory _signature) external payable notPaused notInQueue(msg.sender) {
101    actionInQueue[msg.sender] = ACTION_CLAIM;
102    signatures[msg.sender] = _signature;
103    uint256 amount = 0;
104    emit ClaimInitiated(msg.sender);
105    _sendMessage(ACTION_CLAIM, amount, _signature);
106}
107
108function _nonblockingLzReceive(
109    uint16, /*_srcChainId*/
110    bytes memory, /*_srcAddress*/
111    uint64 _nonce,
112    bytes memory _payload
113) internal override notPaused {
114    require(nonceRegistry[_nonce] == false, "This nonce was already processed");
115    nonceRegistry[_nonce] = true;
116
117    (address payable target, uint256 rewardAmount, uint256 withdrawAmount, bytes memory signature) = abi.decode(_payload, (address, uint256, uint256, bytes));
118
119    require(actionInQueue[target] != bytes32(0x0), "StakingRewardsProxy: No claim or withdrawal is in queue for this address");
120    require(keccak256(signatures[target]) == keccak256(signature), "StakingRewardsProxy: Invalid signature");
121
122    if (withdrawAmount > 0) {
123        require(balances[target] > 0, "StakingRewardsProxy: Invalid withdrawal, no deposits done");
124        require(stakingToken.balanceOf(address(this)) >= withdrawAmount, "StakingRewardsProxy: Insufficient proxy token balance");
125
126        stakingToken.transfer(target, withdrawAmount);
127        balances[target] = 0;
128        emit Withdrawn(target, withdrawAmount);
129    }
130
131    if (rewardAmount > 0) {
132        require(stakingToken.balanceOf(fund) >= rewardAmount, "StakingRewardsProxy: Insufficient fund token balance");
133
134        stakingToken.safeTransferFrom(fund, target, rewardAmount);
135        emit Claimed(target, rewardAmount);
136    }
137
138    delete actionInQueue[target];
139    delete signatures[target];
140}

Recommendation:

We advise the system to simply subtract the withdrawAmount from the balances instead of zeroing the balances entry out ensuring a consistent cross-chain state among the contracts.

Alleviation:

The balance of the user is now properly subtracted from rather than nullified whenever a cross-chain withdrawal is processed ensuring atomicity in the actions that are relayed and correct operation regardless of the order the actions have been submitted in.