Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: concept for accounting on ai agent mechs when staking #128

Merged
merged 14 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/workflow.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:

# Run hardhat coverage and upload codecov report
- name: Solidity coverage summary
run: ./node_modules/.bin/hardhat coverage
run: export NODE_OPTIONS="--max-old-space-size=8192" && ./node_modules/.bin/hardhat coverage
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v1
with:
Expand Down
48 changes: 47 additions & 1 deletion audits/internal4/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ N/A
#### Problems found instrumentally
Several checks are obtained automatically. They are commented. Some issues found need to be fixed. <br>
All automatic warnings are listed in the following file, concerns of which we address in more detail below: <br>
[slither-full](https://github.com/valory-xyz/autonolas-registries/blob/main/audits/internal4/analysis/slither_full.txt)
[slither-full-old](https://github.com/valory-xyz/autonolas-registries/blob/main/audits/internal4/analysis/slither_full_old.txt)

Bad pattern (ref: reentrancy): <br>
```solidity
Expand Down Expand Up @@ -150,12 +150,58 @@ Else without it: ratio = ((serviceNonces[i] - curInfo.nonce) * 1e18) / ts; => DO
2. ref: General considerations #2. We cannot guarantee the absence of manipulation nonce through any checks within the contract. <br>
A reasonable proposal in this case is to provide the opportunity to add a blacklist of services when deploying a staking contract. <br>
This will make it possible to blacklist services with a bad reputation (which is not part of the protocol, but determined by the community). <br>
[x] discussed

#### Simple function needed `isServiceStaked`
[x] fixed

#### _stakingParams.minStakingDeposit
minStakingDeposit must be greater than 1 otherwise it causes confusion with the predefined value of 1.

[x] fixed

### Security issues. Updated 17-10-23
#### Latest CEI fix
```solidity
latest CEI fixup:
IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId);
to
// Add the service Id to the set of staked services
setServiceIds.push(serviceId);
--->
emit ServiceStaked(serviceId, msg.sender, service.multisig, nonces[0]);
This is not a real problem, but will avoid discussions with external auditors.
```
[x] fixed

##### Nonces should now be an array, including events.
```solidity
uint256[] memory nonces = _getMultisigNonces(service.multisig);
icorrect for ServiceStaking*MechUsage
emit ServiceStaked(serviceId, msg.sender, service.multisig, nonces[0]);
->
event ServiceStaked(uint256 indexed serviceId, address indexed owner, address indexed multisig, uint256[] nonce);
emit ServiceStaked(serviceId, msg.sender, service.multisig, nonces);
```
[x] fixed

##### Missing checks on MechAgentMod
```solidity
// If the checkpoint was called in the exact same block, the ratio is zero
if (ts > 0 && curNonces[0] > lastNonces[0]) {
uint256 ratio = ((curNonces[0] - lastNonces[0]) * 1e18) / ts;
ratioPass = (ratio >= livenessRatio);
}

vs

uint256 diffNonces = curNonces[0] - lastNonces[0];
uint256 diffRequestsCounts = curNonces[1] - lastNonces[1];
// Sanity checks for requests counts difference to be at least half of the nonces difference
if (diffRequestsCounts <= diffNonces / 2) {
uint256 ratio = (diffRequestsCounts * 1e18) / ts;
ratioPass = (ratio >= _getLivenessRatio() / 2);
}
```
[x] fixed

Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
// Sources flattened with hardhat v2.17.1 https://hardhat.org

// SPDX-License-Identifier: MIT

// File contracts/multisigs/GnosisSafeSameAddressMultisig.sol

// Original license: SPDX_License_Identifier: MIT
pragma solidity ^0.8.21;

// Gnosis Safe Master Copy interface extracted from the mainnet: https://etherscan.io/address/0xd9db270c1b5e3bd161e8c8503c55ceabee709552#code#F6#L126
interface IGnosisSafe {
/// @dev Gets set of owners.
/// @return Set of Safe owners.
function getOwners() external view returns (address[] memory);

/// @dev Gets threshold.
/// @return Threshold
function getThreshold() external view returns (uint256);
}

/// @dev Zero value when it has to be different from zero.
error ZeroValue();

/// @dev Provided zero address.
error ZeroAddress();

/// @dev Multisig proxy bytecode is not whitelisted.
/// @param multisig Address of a multisig proxy.
error UnauthorizedMultisig(address multisig);

/// @dev Provided incorrect data length.
/// @param expected Expected minimum data length.
/// @param provided Provided data length.
error IncorrectDataLength(uint256 expected, uint256 provided);

/// @dev Provided incorrect multisig threshold.
/// @param expected Expected threshold.
/// @param provided Provided threshold.
error WrongThreshold(uint256 expected, uint256 provided);

/// @dev Provided incorrect number of owners.
/// @param expected Expected number of owners.
/// @param provided Provided number of owners.
error WrongNumOwners(uint256 expected, uint256 provided);

/// @dev Provided incorrect multisig owner.
/// @param provided Provided owner address.
error WrongOwner(address provided);

/// @dev Multisig transaction resulted in a failure.
/// @param provided Provided multisig address.
error MultisigExecFailed(address provided);

/// @title Gnosis Safe Same Address - Smart contract for Gnosis Safe verification of an already existent multisig address.
/// @author Aleksandr Kuperman - <[email protected]>
contract GnosisSafeSameAddressMultisig {
// Default data size to be parsed as an address of a Gnosis Safe multisig proxy address
// This exact size suggests that all the changes to the multisig have been performed and only validation is needed
uint256 public constant DEFAULT_DATA_LENGTH = 20;

// Approved multisig proxy hash
bytes32 public immutable proxyHash;

/// @dev GnosisSafeSameAddressMultisig constructor.
/// @param _proxyHash Approved multisig proxy hash.
constructor(bytes32 _proxyHash) {
if (_proxyHash == bytes32(0)) {
revert ZeroValue();
}

// Record provided multisig proxy bytecode hash
proxyHash = _proxyHash;
}

/// @dev Updates and/or verifies the existent gnosis safe multisig for changed owners and threshold.
/// @notice This function operates with existent multisig proxy that is requested to be updated in terms of
/// the set of owners' addresses and the threshold. There are two scenarios possible:
/// 1. The multisig proxy is already updated before reaching this function. Then the multisig address
/// must be passed as a payload such that its owners and threshold are verified against those specified
/// in the argument list.
/// 2. The multisig proxy is not yet updated. Then the multisig address must be passed in a packed bytes of
/// data along with the Gnosis Safe `execTransaction()` function arguments packed payload. That payload
/// is going to modify the mulsisig proxy as per its signed transaction. At the end, the updated multisig
/// proxy is going to be verified with the provided set of owners' addresses and the threshold.
/// Note that owners' addresses in the multisig are stored in reverse order compared to how they were added:
/// https://etherscan.io/address/0xd9db270c1b5e3bd161e8c8503c55ceabee709552#code#F6#L56
/// @param owners Set of updated multisig owners to verify against.
/// @param threshold Updated number for multisig transaction confirmations.
/// @param data Packed data containing address of an existent gnosis safe multisig and a payload to call the multisig with.
/// @return multisig Address of a multisig (proxy).
function create(
address[] memory owners,
uint256 threshold,
bytes memory data
) external returns (address multisig)
{
// Check for the correct data length
uint256 dataLength = data.length;
if (dataLength < DEFAULT_DATA_LENGTH) {
revert IncorrectDataLength(DEFAULT_DATA_LENGTH, data.length);
}

// Read the proxy multisig address (20 bytes) and the multisig-related data
assembly {
multisig := mload(add(data, DEFAULT_DATA_LENGTH))
}

// Check that the multisig address corresponds to the authorized multisig proxy bytecode hash
bytes32 multisigProxyHash = keccak256(multisig.code);
if (proxyHash != multisigProxyHash) {
revert UnauthorizedMultisig(multisig);
}

// If provided, read the payload that is going to change the multisig ownership and threshold
// The payload is expected to be the `execTransaction()` function call with all its arguments and signature(s)
if (dataLength > DEFAULT_DATA_LENGTH) {
uint256 payloadLength = dataLength - DEFAULT_DATA_LENGTH;
bytes memory payload = new bytes(payloadLength);
for (uint256 i = 0; i < payloadLength; ++i) {
payload[i] = data[i + DEFAULT_DATA_LENGTH];
}

// Call the multisig with the provided payload
(bool success, ) = multisig.call(payload);
if (!success) {
revert MultisigExecFailed(multisig);
}
}

// Get the provided proxy multisig owners and threshold
address[] memory checkOwners = IGnosisSafe(multisig).getOwners();
uint256 checkThreshold = IGnosisSafe(multisig).getThreshold();

// Verify updated multisig proxy for provided owners and threshold
if (threshold != checkThreshold) {
revert WrongThreshold(checkThreshold, threshold);
}
uint256 numOwners = owners.length;
if (numOwners != checkOwners.length) {
revert WrongNumOwners(checkOwners.length, numOwners);
}
// The owners' addresses in the multisig itself are stored in reverse order compared to how they were added:
// https://etherscan.io/address/0xd9db270c1b5e3bd161e8c8503c55ceabee709552#code#F6#L56
// Thus, the check must be carried out accordingly.
for (uint256 i = 0; i < numOwners; ++i) {
if (owners[i] != checkOwners[numOwners - i - 1]) {
revert WrongOwner(owners[i]);
}
}
}
}
87 changes: 87 additions & 0 deletions audits/internal4/analysis/contracts/MechAgentMod-flatten.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Sources flattened with hardhat v2.17.1 https://hardhat.org

// SPDX-License-Identifier: MIT

// File contracts/staking/MechAgentMod.sol

// Original license: SPDX_License_Identifier: MIT
pragma solidity ^0.8.21;

// Multisig interface
interface IMultisig {
/// @dev Gets the multisig nonce.
/// @return Multisig nonce.
function nonce() external view returns (uint256);
}

// AgentMech interface
interface IAgentMech {
/// @dev Gets the requests count for a specific account.
/// @param account Account address.
/// @return requestsCount Requests count.
function getRequestsCount(address account) external view returns (uint256 requestsCount);
}

/// @dev Provided zero mech agent address.
error ZeroMechAgentAddress();

/// @title MechAgentMod - Abstract smart contract for AI agent mech staking modification
/// @author Aleksandr Kuperman - <[email protected]>
/// @author Andrey Lebedev - <[email protected]>
/// @author Mariapia Moscatiello - <[email protected]>
abstract contract MechAgentMod {
// AI agent mech contract address.
address public immutable agentMech;

/// @dev MechAgentMod constructor.
/// @param _agentMech AI agent mech contract address.
constructor(address _agentMech) {
if (_agentMech == address(0)) {
revert ZeroMechAgentAddress();
}
agentMech = _agentMech;
}

/// @dev Gets service multisig nonces.
/// @param multisig Service multisig address.
/// @return nonces Set of one or more service multisig nonces depending on implementation.
function _getMultisigNonces(address multisig) internal view virtual returns (uint256[] memory nonces) {
nonces = new uint256[](2);
nonces[0] = IMultisig(multisig).nonce();
nonces[1] = IAgentMech(agentMech).getRequestsCount(multisig);
}

/// @dev Gets the liveness ratio.
/// @return Liveness ratio.
function _getLivenessRatio() internal view virtual returns (uint256);

/// @dev Checks if the service multisig liveness ratio passes the defined liveness threshold.
/// @notice The formula for calculating the ratio is the following:
/// currentNonces - [service multisig nonce at time now (block.timestamp), requests count at time now];
/// lastNonces - [service multisig nonce at the previous checkpoint or staking time (tsStart), requests count at time tsStart];
/// Requests count difference must be at least two times smaller than the nonce difference:
/// (currentNonces[1] - lastNonces[1]) <= (currentNonces[0] - lastNonces[0]) / 2;
/// ratio = (currentNonces[1] - lastNonce[1]) / (block.timestamp - tsStart).
/// Liveness ratio for mech requests count:
/// ratio >= counterRequestsRatio, where counterRequestsRatio = livenessRatio / 2,
/// since there is at least one tx for sending a request to AgentMech, and another tx for its subsequent execution.
/// @param curNonces Current service multisig nonces.
/// @param lastNonces Last service multisig nonces.
/// @param ts Time difference between current and last timestamps.
/// @return ratioPass True, if the liveness ratio passes the check.
function _isRatioPass(
uint256[] memory curNonces,
uint256[] memory lastNonces,
uint256 ts
) internal view virtual returns (bool ratioPass)
{
uint256 diffNonces = curNonces[0] - lastNonces[0];
uint256 diffRequestsCounts = curNonces[1] - lastNonces[1];

// Sanity checks for requests counts difference to be at least half of the nonces difference
if (diffRequestsCounts <= diffNonces / 2) {
uint256 ratio = (diffRequestsCounts * 1e18) / ts;
ratioPass = (ratio >= _getLivenessRatio() / 2);
}
}
}
Loading
Loading