Omniscia Dapp Radar Audit

StakingRewardsController Manual Review Findings

StakingRewardsController Manual Review Findings

SRC-01M: Inexistent Synchronization of Pool Rewards

Description:

The setRewardPerSecond function does not update the accumulated pool rewards up to the point the change is occurring, thereby allowing the owner to nullify otherwise expected rewards for the users.

Impact:

In the current implementation, the rewardPerSecond change will retroactively apply for all elapsed seconds since the last update which is an undesirable trait.

Example:

contracts/StakingRewardsController.sol
213/// @notice Sets the token per second to be distributed. Can only be called by the owner.
214/// @param _rewardPerSecond The amount of token to be distributed per second.
215function setRewardPerSecond(uint256 _rewardPerSecond) public onlyOwner {
216    rewardPerSecond = _rewardPerSecond;
217    emit LogRewardPerSecond(_rewardPerSecond);
218}

Recommendation:

We advise the updatePool function to be invoked prior to the adjustment of the reward per second variable similarly to other staking protocols.

Alleviation:

The updatePool function is now properly invoked prior to the adjustment of the rewardPerSecond variable properly accounting for previously elapsed reward amounts.

SRC-02M: Hard-Coded Gas Allowance for Cross-Chain Relay

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:

contracts/StakingRewardsController.sol
145// use adapterParams v1 to specify more gas for the destination
146uint16 version = 1;
147uint256 gasForDestinationLzReceive = 250000;
148bytes memory adapterParams = abi.encodePacked(version, gasForDestinationLzReceive);
149

150// get the fees we need to pay to LayerZero for message delivery
151(uint256 messageFee, ) = lzEndpoint.estimateFees(_dstChain, address(this), payload, false, adapterParams);
152

153require(address(this).balance >= messageFee, "StakingRewardsController: address(this).balance < messageFee. Fund this contract");
154

155_lzSend( // {value: messageFee} will be paid out of this contract!
156    _dstChain, // destination chainId
157    payload, // abi.encode()'ed bytes
158    payable(address(this)), // refund address (LayerZero will refund any extra gas)
159    address(0x0), // future param, unused for this example
160    adapterParams // v1 adapterParams, specify custom destination gas qty
161);

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 two actions supported by the proxy (withdraw & claim) to be configurable to an arbitrary level thereby alleviating this exhibit in full.

SRC-03M: Non-Standard Signature Verification Workflow

Description:

The signature verification performed by the _nonblockingLzReceive function uses the SignatureChecker implementation by OpenZeppelin which in turn implements the EIP-1271 standard that contains an external call.

Impact:

The code currently performs an external call to the user on each cross-chain transaction for that particular account which could theoretically yield false and thus block the account from interacting with the protocol improperly.

Example:

contracts/StakingRewardsController.sol
166function _nonblockingLzReceive(
167    uint16 _srcChainId,
168    bytes memory _srcAddress,
169    uint64 _nonce,
170    bytes memory _payload
171) internal override {
172    require(nonceRegistry[_nonce] == false, "This nonce was already processed");
173    nonceRegistry[_nonce] = true;
174

175    // use assembly to extract the address from the bytes memory parameter
176    address sendBackToAddress;
177    assembly {
178        sendBackToAddress := mload(add(_srcAddress, 20))
179    }
180

181    (address user, bytes32 action, uint256 amount, bytes memory signature) = abi.decode(_payload, (address, bytes32, uint256, bytes));
182    require(action == ACTION_STAKE || action == ACTION_WITHDRAW || action == ACTION_CLAIM, "StakingRewardsController: Invalid action");
183

184    bytes32 amountHashed = keccak256(abi.encodePacked(stringToBytes32(Strings.toString(amount))));
185    bytes32 hash = keccak256(abi.encodePacked(action, amountHashed));
186    require(SignatureChecker.isValidSignatureNow(user, hash, signature), "StakingRewardsController: Invalid signature");
187

188    if (action == ACTION_STAKE) {
189        _stake(user, amount, _srcChainId);
190    } else if (action == ACTION_WITHDRAW) {
191        _withdraw(user, signature, _srcChainId, sendBackToAddress);
192    } else if (action == ACTION_CLAIM) {
193        _claim(user, signature, _srcChainId, sendBackToAddress);
194    }
195}

Recommendation:

We advise the function to perform simple signature validation as the current signature scheme can lead to signatures that expire and thus block the cross-chain receival flow of the controller from a cross-chain transaction.

Alleviation:

A simple ECDSA based signature validation is now performed in place of the complex EIP-1271 mechanism that was previously imposed thereby alleviating this exhibit.

SRC-04M: Improper Withdrawal Debt Calculation

Description:

The rewardDebt calculation performed during withdrawals is incorrect and causes rewards to be miscalculated for any user that withdraws as the _getReward call that relays the withdrawal does not properly update the rewardDebt as it does not account for the _withdrawalAmount in its calculation.

Impact:

The current calculations do not properly update the rewardDebt thereby breaking the reward accounting system of the controller.

Example:

contracts/StakingRewardsController.sol
107function _withdraw(address _user, bytes memory _signature, uint16 _dstChain, address _dstAddress) internal nonReentrant {
108    uint256 userBalance = userInfo[_user].amountPerChain[_dstChain];
109    require(userBalance > 0, "RadarStakingRewards: this wallet has nothing staked on this chain");
110

111    _getReward(_user, userBalance, _signature, _dstChain, _dstAddress);
112

113    totalSupply -= userBalance;
114

115    UserInfo storage user = userInfo[_user];
116    user.amount -= userBalance;
117    user.amountPerChain[_dstChain] = 0;
118

119    emit Withdrawn(_user, userBalance, _dstChain);
120}
121

122function _claim(address _user, bytes memory _signature, uint16 _dstChain, address _dstAddress) internal nonReentrant {
123    _getReward(_user, 0, _signature, _dstChain, _dstAddress);
124}
125

126function _getReward(address _user, uint256 _withdrawalAmount, bytes memory _signature, uint16 _dstChain, address _dstAddress) private {
127    PoolInfo memory pool = updatePool();
128    UserInfo storage user = userInfo[_user];
129    uint256 rewardAmount;
130    if (user.amount > 0) {
131        rewardAmount = (user.amount * pool.accToken1PerShare / BASE_UNIT) - user.rewardDebt + user.unpaidRewards;
132        user.unpaidRewards = 0;
133    }
134    user.rewardDebt = userInfo[_user].amount * pool.accToken1PerShare / BASE_UNIT;
135

136    _sendMessage(_user, rewardAmount, _withdrawalAmount, _signature, _dstChain, _dstAddress);
137    emit Claimed(_user, rewardAmount, _dstChain);
138}

Recommendation:

We advise the userInfo[_user].amount variable used in the rewardDebt calculation to have the _withdrawalAmount subtracted ensuring the debt is calculated properly and rewards are distributed evenly.

Alleviation:

The withdrawal amount is now properly subtracted from the amount the debt is calculated from ensuring that it operates as expected.

Prev

LzApp.sol (LAP-M)

Next

StakingRewardsProxy.sol (SRP-M)

On this page