Skip to content

Commit

Permalink
Merge pull request #71 from valory-xyz/dual_token_testing
Browse files Browse the repository at this point in the history 
refactor: dual token is onwerless, naming correction
  • Loading branch information
@kupermind
kupermind authored Feb 25, 2025
2 parents fb46134 + 7ca5d38 commit 1f3ba3a
Show file tree
Hide file tree
Showing 5 changed files with 896 additions and 78 deletions.
4 changes: 4 additions & 0 deletions audits/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,7 @@ contracts is located in this folder: [internal audit 1](https://github.com/valor
An internal audit with a focus on fixing contribute contracts
contracts is located in this folder: [internal audit 2](https://github.com/valory-xyz/autonolas-staking-programmes/blob/main/audits/internal2).

### Internal audit
An internal audit with a focus on dual token contracts
contracts is located in this folder: [internal audit 3](https://github.com/valory-xyz/autonolas-staking-programmes/blob/main/audits/internal3).

117 changes: 117 additions & 0 deletions audits/internal3/README.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# Internal audit of autonolas-staking-programmes
The review has been performed based on the contract code in the following repository:<br>
`https://github.com/valory-xyz/autonolas-staking-programmes` <br>
commit: `v1.6.0-pre-internal-audit` or `710ee320d06acd0ee793fa13677987fb347dffdc` <br>

## Objectives
The audit focused on contracts in this repo. <br>

### ERC20/ERC721 checks
N/A

### Security issues.

#### Notes: DualStakingToken named revert
```
if (stakerInfo.account != address(0)) {
revert();
}
if (stakerInfo.account == address(0)) {
revert();
}
if (stakingState != IStaking.StakingState.Evicted) {
revert();
}
// Check for staker existence
if (stakerInfo.account == address(0)) {
revert();
}
```
[]

#### Notes: One way deposit(). Should there be an opposite function or would overcomplicate the logic?
```
function deposit(uint256 amount) external {} <- can't redo deposit()
```
[]

#### Notes: reward in both token? triple check and test logic
```
Why does the code only show rewards in second tokens? Shouldn't it be paid out symmetrically in OLAS/second ERC20 token?
```
[]

#### Notes: reward in second token. triple check and test logic
```
Why do we send a reward equal stakerInfo.reward to the second tokens? Why not stakerInfo.reward * ratio?
uint256 reward = stakerInfo.reward;
...
// Transfer second token reward to the service multisig
if (reward > 0) {
_withdraw(multisig, reward); <--- mul ratio?
}
```
[]

#### Notes: Available reward in second token. triple check and tests logic
```
The logic looks confusing and somewhere it multiplies by ratio, sometime not. Please, triple check and test code!
if (numServices > 0) {
uint256 lastAvailableRewards = availableRewards;
uint256 totalRewards;
for (uint256 i = 0; i < numServices; ++i) {
totalRewards += eligibleServiceRewards[i]; --> totalRewards += eligibleServiceRewards[i] --> without ratio
}
uint256 curServiceId;
// If total allocated rewards are not enough, adjust the reward value
if ((totalRewards * rewardRatio) / 1e18 > lastAvailableRewards) { --> totalRewards * rewardRatio vs lastAvailableRewards
// Traverse all the eligible services and adjust their rewards proportional to leftovers
// Note the algorithm is the exact copy of StakingBase logic
uint256 updatedReward;
uint256 updatedTotalRewards;
for (uint256 i = 1; i < numServices; ++i) {
// Calculate the updated reward
updatedReward = (eligibleServiceRewards[i] * lastAvailableRewards) / totalRewards;
// Add to the total updated reward
updatedTotalRewards += updatedReward;
curServiceId = eligibleServiceIds[i];
// Add reward to the overall service reward
mapStakerInfos[curServiceId].reward += (updatedReward * rewardRatio) / 1e18;
}
// Process the first service in the set
updatedReward = (eligibleServiceRewards[0] * lastAvailableRewards) / totalRewards;
updatedTotalRewards += updatedReward;
curServiceId = eligibleServiceIds[0];
// If the reward adjustment happened to have small leftovers, add it to the first service
if (lastAvailableRewards > updatedTotalRewards) {
updatedReward += lastAvailableRewards - updatedTotalRewards;
}
// Add reward to the overall service reward
mapStakerInfos[curServiceId].reward += (updatedReward * rewardRatio) / 1e18;
// Set available rewards to zero
lastAvailableRewards = 0;
} else {
// Traverse all the eligible services and add to their rewards
for (uint256 i = 0; i < numServices; ++i) {
// Add reward to the service overall reward
curServiceId = eligibleServiceIds[i];
mapStakerInfos[curServiceId].reward += (eligibleServiceRewards[i] * rewardRatio) / 1e18;
}
// Adjust available rewards
lastAvailableRewards -= totalRewards; --> lastAvailableRewards vs totalRewards without ratio
}
// Update the storage value of available rewards
availableRewards = lastAvailableRewards; --> lastAvailableRewards vs totalRewards without ratio
}
```
[]




113 changes: 37 additions & 76 deletions contracts/dual_token/DualStakingToken.sol
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;

import {ERC721TokenReceiver} from "../../lib/autonolas-registries/lib/solmate/src/tokens/ERC721.sol";
import {IService} from "./interfaces/IService.sol";
import {IStaking} from "./interfaces/IStaking.sol";
import {SafeTransferLib} from "../libraries/SafeTransferLib.sol";
Expand Down Expand Up @@ -28,7 +29,7 @@ error OwnerOnly(address sender, address owner);
error ReentrancyGuard();

struct StakerInfo {
// Staking token amount
// Second token amount
uint256 stakingAmount;
// Cumulative reward
uint256 reward;
Expand All @@ -40,29 +41,27 @@ struct StakerInfo {
/// @author Aleksandr Kuperman - <[email protected]>
/// @author Andrey Lebedev - <[email protected]>
/// @author Mariapia Moscatiello - <[email protected]>
contract DualStakingToken {
contract DualStakingToken is ERC721TokenReceiver {
event OwnerUpdated(address indexed owner);
event StakingTokenParamsUpdated(uint256 stakingTokenAmount, uint256 rewardRatio);
event StakingTokenParamsUpdated(uint256 secondTokenAmount, uint256 rewardRatio);
event Deposit(address indexed sender, uint256 amount, uint256 balance, uint256 availableRewards);
event Withdraw(address indexed to, uint256 amount);

// Service registry address
address public immutable serviceRegistry;
// Staking token address (except for OLAS)
address public immutable stakingToken;
// Service staking instance address
// Second token address (except for OLAS)
address public immutable secondToken;
// OLAS service staking instance address
address public immutable stakingInstance;
// Required second token amount
uint256 public immutable secondTokenAmount;
// Second token ratio to OLAS rewards in 1e18 form
uint256 public immutable rewardRatio;

// Required staking token amount
uint256 public stakingTokenAmount;
// Staking token ratio to OLAS rewards in 1e18 form
uint256 public rewardRatio;
// Staking token contract balance
// Second token contract balance
uint256 public balance;
// Staking token available rewards
// Second token available rewards
uint256 public availableRewards;
// Owner address
address public owner;

// Reentrancy lock
uint256 internal _locked = 1;
Expand All @@ -74,17 +73,17 @@ contract DualStakingToken {

/// @dev DualStakingToken constructor.
/// @param _serviceRegistry Service registry address.
/// @param _stakingToken Staking token address.
/// @param _secondToken Second token address.
/// @param _stakingInstance Service staking instance address.
/// @param _rewardRatio Staking token ratio to OLAS rewards in 1e18 form.
/// @param _rewardRatio Second token ratio to OLAS rewards in 1e18 form.
constructor(
address _serviceRegistry,
address _stakingToken,
address _secondToken,
address _stakingInstance,
uint256 _rewardRatio
) {
// Check for zero addresses
if (_serviceRegistry == address(0) || _stakingToken == address(0) || _stakingInstance == address(0)) {
if (_serviceRegistry == address(0) || _secondToken == address(0) || _stakingInstance == address(0)) {
revert ZeroAddress();
}

Expand All @@ -94,18 +93,15 @@ contract DualStakingToken {
}

serviceRegistry = _serviceRegistry;
stakingToken = _stakingToken;
secondToken = _secondToken;
stakingInstance = _stakingInstance;

rewardRatio = _rewardRatio;

// Calculate staking token amount based on staking instance service information
// Calculate second token amount based on staking instance service information
uint256 numAgentInstances = IStaking(_stakingInstance).numAgentInstances();
uint256 minStakingDeposit = IStaking(_stakingInstance).minStakingDeposit();
// Total service deposit = minStakingDeposit + minStakingDeposit * numAgentInstances
stakingTokenAmount = (minStakingDeposit * (1 + numAgentInstances) * _rewardRatio) / 1e18;

owner = msg.sender;
secondTokenAmount = (minStakingDeposit * (1 + numAgentInstances) * _rewardRatio) / 1e18;
}

/// @dev Withdraws the reward amount to a service owner.
Expand All @@ -116,49 +112,12 @@ contract DualStakingToken {
// Update the contract balance
balance -= amount;

SafeTransferLib.safeTransfer(stakingToken, to, amount);
SafeTransferLib.safeTransfer(secondToken, to, amount);

emit Withdraw(to, amount);
}

/// @dev Changes contract owner address.
/// @param newOwner Address of a new owner.
function changeOwner(address newOwner) external {
// Check for the ownership
if (msg.sender != owner) {
revert OwnerOnly(msg.sender, owner);
}

// Check for the zero address
if (newOwner == address(0)) {
revert ZeroAddress();
}

owner = newOwner;
emit OwnerUpdated(newOwner);
}

/// @dev Changes staking token params.
/// @param newStakingTokenAmount New staking token amount.
/// @param newRewardRatio New staking token ratio to OLAS rewards in 1e18 form.
function changeStakingTokenParams(uint256 newStakingTokenAmount, uint256 newRewardRatio) external {
// Check for the ownership
if (msg.sender != owner) {
revert OwnerOnly(msg.sender, owner);
}

// Check for zero values
if (newStakingTokenAmount == 0 || newRewardRatio == 0) {
revert ZeroValue();
}

stakingTokenAmount = newStakingTokenAmount;
rewardRatio = newRewardRatio;

emit StakingTokenParamsUpdated(newStakingTokenAmount, newRewardRatio);
}

/// @dev Stakes OLAS service Id and required staking token amount.
/// @dev Stakes OLAS service Id and required second token amount.
/// @param serviceId OLAS driven service Id.
function stake(uint256 serviceId) external {
// Reentrancy guard
Expand All @@ -167,13 +126,17 @@ contract DualStakingToken {
}
_locked = 2;

if (availableRewards == 0) {
revert ZeroValue();
}

StakerInfo storage stakerInfo = mapStakerInfos[serviceId];
// Check for existing staker
if (stakerInfo.account != address(0)) {
revert();
}

uint256 amount = stakingTokenAmount;
uint256 amount = secondTokenAmount;

// Record staker info values
stakerInfo.account = msg.sender;
Expand All @@ -184,7 +147,7 @@ contract DualStakingToken {

mapMutisigs[multisig] = true;

SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);
SafeTransferLib.safeTransferFrom(secondToken, msg.sender, address(this), amount);

IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId);

Expand Down Expand Up @@ -215,10 +178,10 @@ contract DualStakingToken {
IStaking(stakingInstance).checkpoint();

// Process rewards
// If there are eligible services, calculate and update staking token rewards
// If there are eligible services, calculate and update second token rewards
uint256 numServices = eligibleServiceIds.length;
if (numServices > 0) {
uint256 lastAvailableRewards = availableRewards;
uint256 lastAvailableRewards = availableRewards;
if (numServices > 0 && lastAvailableRewards > 0) {
uint256 totalRewards;
for (uint256 i = 0; i < numServices; ++i) {
totalRewards += eligibleServiceRewards[i];
Expand Down Expand Up @@ -308,7 +271,7 @@ contract DualStakingToken {
_locked = 1;
}

/// @dev Unstakes OLAS service Id and unbonds staking token amount.
/// @dev Unstakes OLAS service Id and unbonds second token amount.
/// @param serviceId OLAS driven service Id.
function unstake(uint256 serviceId) external {
// Reentrancy guard
Expand Down Expand Up @@ -343,11 +306,10 @@ contract DualStakingToken {
delete mapStakerInfos[serviceId];
delete mapMutisigs[multisig];

// Transfer staking token amount back to the staker
// Transfer second token amount back to the staker
_withdraw(account, stakingAmount);

// TODO Reward is sent to the service multisig or staker account?
// Transfer staking token reward to the service multisig
// Transfer second token reward to the service multisig
if (reward > 0) {
_withdraw(multisig, reward);
}
Expand All @@ -364,7 +326,7 @@ contract DualStakingToken {
_locked = 1;
}

/// @dev Claims OLAS and staking token rewards.
/// @dev Claims OLAS and second token rewards.
/// @param serviceId OLAS driven service Id.
function claim(uint256 serviceId) external {
// Reentrancy guard
Expand All @@ -388,8 +350,7 @@ contract DualStakingToken {
// Get service multisig
(, address multisig, , , , , ) = IService(serviceRegistry).mapServices(serviceId);

// TODO Reward is sent to the service multisig or staker account?
// Transfer staking token reward to the service multisig
// Transfer second token reward to the service multisig
if (reward > 0) {
_withdraw(multisig, reward);
}
Expand Down Expand Up @@ -418,7 +379,7 @@ contract DualStakingToken {
availableRewards = newAvailableRewards;

// Add to the overall balance
SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);
SafeTransferLib.safeTransferFrom(secondToken, msg.sender, address(this), amount);

emit Deposit(msg.sender, amount, newBalance, newAvailableRewards);

Expand Down
4 changes: 2 additions & 2 deletions contracts/dual_token/DualStakingTokenActivityChecker.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ contract DualStakingTokenActivityChecker {
/// @dev Gets service multisig nonces.
/// @param multisig Service multisig address.
/// @return nonces Set of a single service multisig nonce.
function getMultisigNonces(address multisig) external view virtual returns (uint256[] memory nonces) {
function getMultisigNonces(address multisig) external view returns (uint256[] memory nonces) {
// Check for token duality
// This check prevents from staking directly to service staking contract without locking the staking token as well
if (!IDualStakingToken(dualStakingToken).mapMutisigs(multisig)) {
Expand Down Expand Up @@ -135,7 +135,7 @@ contract DualStakingTokenActivityChecker {
uint256[] memory curNonces,
uint256[] memory lastNonces,
uint256 ts
) external view virtual returns (bool ratioPass) {
) external view returns (bool ratioPass) {
// Check for lock
// This lock prevents checkpoint being called directly from service staking contract
if (_locked == 2) {
Expand Down
Loading

0 comments on commit 1f3ba3a

Please sign in to comment.