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

Initial setup for testnet-delegation contract #427

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

Malak67
Copy link
Contributor

@Malak67 Malak67 commented Mar 27, 2023

No description provided.

@VargaElod23 VargaElod23 linked an issue Apr 7, 2023 that may be closed by this pull request
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@rattrap rattrap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also check the internal validators balances before delegation to them. If they already have 2t+1, we don't need to delegate when registering new external validator

@VargaElod23 VargaElod23 requested a review from rattrap April 19, 2023 16:50
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrf_key,

Check warning

Code scanning / Slither

Conformance to Solidity naming conventions

Parameter IDPOS.registerValidator(address,bytes,bytes,uint16,string,string).vrf_key (packages/testnet-delegation/contracts/interfaces/IDPOS.sol#109) is not in mixedCase
function cancelUndelegate(address validator) external;

// Redelegates <amount> of tokens from one validator to the other
function reDelegate(address validator_from, address validator_to, uint256 amount) external;

Check warning

Code scanning / Slither

Conformance to Solidity naming conventions

Parameter IDPOS.reDelegate(address,address,uint256).validator_from (packages/testnet-delegation/contracts/interfaces/IDPOS.sol#89) is not in mixedCase
function cancelUndelegate(address validator) external;

// Redelegates <amount> of tokens from one validator to the other
function reDelegate(address validator_from, address validator_to, uint256 amount) external;

Check warning

Code scanning / Slither

Conformance to Solidity naming conventions

Parameter IDPOS.reDelegate(address,address,uint256).validator_to (packages/testnet-delegation/contracts/interfaces/IDPOS.sol#89) is not in mixedCase
internalStake += internalValidatorInfo.total_stake;
}
if (index < externalValidators.length) {
IDPOS.ValidatorBasicInfo memory externalValidatorInfo = dpos.getValidator(externalValidators[index]);

Check warning

Code scanning / Slither

Variable names too similar

Variable DelegationOrchestrator._calculateStakes().externalValidatorInfo (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#133) is too similar to DelegationOrchestrator._calculateStakes().internalValidatorInfo (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#129)
Comment on lines +28 to +51
function _delegate(address implementation) internal virtual returns (bytes memory returnData) {
assembly {
// Copy msg.data. We take full control of memory in this inline assembly
// block because it will not return to Solidity code. We overwrite the
// Solidity scratch pad at memory position 0.
calldatacopy(0, 0, calldatasize())

// Call the implementation.
// out and outsize are 0 because we don't know the size yet.
let result := call(gas(), implementation, callvalue(), 0, calldatasize(), 0, 0)

// Copy the returned data.
returndatacopy(0, 0, returndatasize())

switch result
// delegatecall returns 0 on error.
case 0 {
revert(0, returndatasize())
}
default {
return(0, returndatasize())
}
}
}

Check warning

Code scanning / Slither

Assembly usage

CallProxy._delegate(address) (packages/testnet-delegation/contracts/CallProxy.sol#28-51) uses assembly - INLINE ASM (packages/testnet-delegation/contracts/CallProxy.sol#29-50)
// @note registrar of indexes+1 of validators in the internalValidators
mapping(address => uint256) public internalValidatorRegistered;
// @note registrar of indexes+1 of validators in the externalValidators
mapping(address => uint256) public externalValidatorRegistered;

Check warning

Code scanning / Slither

Variable names too similar

Variable DelegationOrchestrator.externalValidatorRegistered (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#29) is too similar to DelegationOrchestrator.internalValidatorRegistered (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#27)
@VargaElod23
Copy link
Contributor

Implement the following shadowings:

// Returns validator basic info (everything except list of his delegators)
function getValidator(address validator)
external
view
returns (ValidatorBasicInfo memory validator_info);

function getValidators(uint32 batch)
    external
    view
    returns (ValidatorData[] memory validators, bool end);

/**
 * @notice Returns list of validators owned by an address
 *
 * @param owner        Owner address
 * @param batch        Batch number to be fetched. If the list is too big it cannot return all validators in one call. Instead, users are fetching batches of 100 account at a time
 *
 * @return validators  Batch of N validators basic info
 * @return end         Flag if there are no more accounts left. To get all accounts, caller should fetch all batches until he sees end == true
 **/
function getValidatorsFor(address owner, uint32 batch)
    external
    view
    returns (ValidatorData[] memory validators, bool end);

Comment on lines 109 to 164
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrfKey,
uint16 commission,
string calldata description,
string calldata endpoint
) external payable override nonReentrant {
uint256 delegationValue = (2 * msg.value) / internalValidators.length;
require(
externalValidatorRegistered[validator] == 0 && internalValidatorRegistered[validator] == 0,
"Validator already registered"
);
require(msg.value >= (MIN_REGISTRATION_DELEGATION), "Sent value less than minimal delegation for registration");
require(
address(this).balance >= delegationValue * internalValidators.length,
"Insufficient funds in contract for testnet rebalance"
);
uint256 internalStake = 0;
uint256 externalStake = 0;
(internalStake, externalStake) = _calculateStakes();
require(internalStake != 0 || externalStake != 0, "Calculating current stakes failed!");
uint256 majorityStake = 2 * externalStake + msg.value + MIN_REGISTRATION_DELEGATION;
if (internalStake < majorityStake) {
// Delegate 2*tokens to our validators
for (uint8 i = 0; i < internalValidators.length; ) {
(bool hasDelegated, ) = address(dpos).call{value: delegationValue}(
abi.encodeWithSignature("delegate(address)", internalValidators[i])
);

require(hasDelegated, "Delegate call failed");
emit InternalValidatorDelegationIncreased(internalValidators[i], delegationValue);
unchecked {
++i;
}
}
}
externalValidatorsByOwners[msg.sender].push(validator);
externalValidators.push(validator);
externalValidatorRegistered[validator] = externalValidators.length;
externalValidatorOwners[validator] = msg.sender;
bytes memory input = abi.encodeWithSelector(
REGISTER_VALIDATOR_SELECTOR,
validator,
proof,
vrfKey,
commission,
description,
endpoint
);
(bool hasRegistered, ) = address(dpos).call{value: msg.value}(input);

require(hasRegistered, "Register validator call failed");

emit ExternalValidatorRegistered(validator, msg.sender, msg.value);
}

Check failure

Code scanning / Slither

Reentrancy vulnerabilities

Reentrancy in DelegationOrchestrator.registerValidator(address,bytes,bytes,uint16,string,string) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#109-164): External calls: - (hasDelegated) = address(dpos).call{value: delegationValue}(abi.encodeWithSignature(delegate(address),internalValidators[i])) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#135-137) State variables written after the call(s): - externalValidatorRegistered[validator] = externalValidators.length (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#148) DelegationOrchestrator.externalValidatorRegistered (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#30) can be used in cross function reentrancies: - DelegationOrchestrator.externalValidatorRegistered (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#30) - externalValidators.push(validator) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#147) DelegationOrchestrator.externalValidators (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#22) can be used in cross function reentrancies: - DelegationOrchestrator.externalValidators (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#22)
Comment on lines 109 to 164
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrfKey,
uint16 commission,
string calldata description,
string calldata endpoint
) external payable override nonReentrant {
uint256 delegationValue = (2 * msg.value) / internalValidators.length;
require(
externalValidatorRegistered[validator] == 0 && internalValidatorRegistered[validator] == 0,
"Validator already registered"
);
require(msg.value >= (MIN_REGISTRATION_DELEGATION), "Sent value less than minimal delegation for registration");
require(
address(this).balance >= delegationValue * internalValidators.length,
"Insufficient funds in contract for testnet rebalance"
);
uint256 internalStake = 0;
uint256 externalStake = 0;
(internalStake, externalStake) = _calculateStakes();
require(internalStake != 0 || externalStake != 0, "Calculating current stakes failed!");
uint256 majorityStake = 2 * externalStake + msg.value + MIN_REGISTRATION_DELEGATION;
if (internalStake < majorityStake) {
// Delegate 2*tokens to our validators
for (uint8 i = 0; i < internalValidators.length; ) {
(bool hasDelegated, ) = address(dpos).call{value: delegationValue}(
abi.encodeWithSignature("delegate(address)", internalValidators[i])
);

require(hasDelegated, "Delegate call failed");
emit InternalValidatorDelegationIncreased(internalValidators[i], delegationValue);
unchecked {
++i;
}
}
}
externalValidatorsByOwners[msg.sender].push(validator);
externalValidators.push(validator);
externalValidatorRegistered[validator] = externalValidators.length;
externalValidatorOwners[validator] = msg.sender;
bytes memory input = abi.encodeWithSelector(
REGISTER_VALIDATOR_SELECTOR,
validator,
proof,
vrfKey,
commission,
description,
endpoint
);
(bool hasRegistered, ) = address(dpos).call{value: msg.value}(input);

require(hasRegistered, "Register validator call failed");

emit ExternalValidatorRegistered(validator, msg.sender, msg.value);
}

Check warning

Code scanning / Slither

Divide before multiply

DelegationOrchestrator.registerValidator(address,bytes,bytes,uint16,string,string) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#109-164) performs a multiplication on the result of a division: - delegationValue = (2 * msg.value) / internalValidators.length (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#117) - require(bool,string)(address(this).balance >= delegationValue * internalValidators.length,Insufficient funds in contract for testnet rebalance) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#123-126)
Comment on lines 109 to 164
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrfKey,
uint16 commission,
string calldata description,
string calldata endpoint
) external payable override nonReentrant {
uint256 delegationValue = (2 * msg.value) / internalValidators.length;
require(
externalValidatorRegistered[validator] == 0 && internalValidatorRegistered[validator] == 0,
"Validator already registered"
);
require(msg.value >= (MIN_REGISTRATION_DELEGATION), "Sent value less than minimal delegation for registration");
require(
address(this).balance >= delegationValue * internalValidators.length,
"Insufficient funds in contract for testnet rebalance"
);
uint256 internalStake = 0;
uint256 externalStake = 0;
(internalStake, externalStake) = _calculateStakes();
require(internalStake != 0 || externalStake != 0, "Calculating current stakes failed!");
uint256 majorityStake = 2 * externalStake + msg.value + MIN_REGISTRATION_DELEGATION;
if (internalStake < majorityStake) {
// Delegate 2*tokens to our validators
for (uint8 i = 0; i < internalValidators.length; ) {
(bool hasDelegated, ) = address(dpos).call{value: delegationValue}(
abi.encodeWithSignature("delegate(address)", internalValidators[i])
);

require(hasDelegated, "Delegate call failed");
emit InternalValidatorDelegationIncreased(internalValidators[i], delegationValue);
unchecked {
++i;
}
}
}
externalValidatorsByOwners[msg.sender].push(validator);
externalValidators.push(validator);
externalValidatorRegistered[validator] = externalValidators.length;
externalValidatorOwners[validator] = msg.sender;
bytes memory input = abi.encodeWithSelector(
REGISTER_VALIDATOR_SELECTOR,
validator,
proof,
vrfKey,
commission,
description,
endpoint
);
(bool hasRegistered, ) = address(dpos).call{value: msg.value}(input);

require(hasRegistered, "Register validator call failed");

emit ExternalValidatorRegistered(validator, msg.sender, msg.value);
}

Check notice

Code scanning / Slither

Calls inside a loop

DelegationOrchestrator.registerValidator(address,bytes,bytes,uint16,string,string) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#109-164) has external calls inside a loop: (hasDelegated) = address(dpos).call{value: delegationValue}(abi.encodeWithSignature(delegate(address),internalValidators[i])) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#135-137)
Comment on lines 109 to 164
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrfKey,
uint16 commission,
string calldata description,
string calldata endpoint
) external payable override nonReentrant {
uint256 delegationValue = (2 * msg.value) / internalValidators.length;
require(
externalValidatorRegistered[validator] == 0 && internalValidatorRegistered[validator] == 0,
"Validator already registered"
);
require(msg.value >= (MIN_REGISTRATION_DELEGATION), "Sent value less than minimal delegation for registration");
require(
address(this).balance >= delegationValue * internalValidators.length,
"Insufficient funds in contract for testnet rebalance"
);
uint256 internalStake = 0;
uint256 externalStake = 0;
(internalStake, externalStake) = _calculateStakes();
require(internalStake != 0 || externalStake != 0, "Calculating current stakes failed!");
uint256 majorityStake = 2 * externalStake + msg.value + MIN_REGISTRATION_DELEGATION;
if (internalStake < majorityStake) {
// Delegate 2*tokens to our validators
for (uint8 i = 0; i < internalValidators.length; ) {
(bool hasDelegated, ) = address(dpos).call{value: delegationValue}(
abi.encodeWithSignature("delegate(address)", internalValidators[i])
);

require(hasDelegated, "Delegate call failed");
emit InternalValidatorDelegationIncreased(internalValidators[i], delegationValue);
unchecked {
++i;
}
}
}
externalValidatorsByOwners[msg.sender].push(validator);
externalValidators.push(validator);
externalValidatorRegistered[validator] = externalValidators.length;
externalValidatorOwners[validator] = msg.sender;
bytes memory input = abi.encodeWithSelector(
REGISTER_VALIDATOR_SELECTOR,
validator,
proof,
vrfKey,
commission,
description,
endpoint
);
(bool hasRegistered, ) = address(dpos).call{value: msg.value}(input);

require(hasRegistered, "Register validator call failed");

emit ExternalValidatorRegistered(validator, msg.sender, msg.value);
}

Check notice

Code scanning / Slither

Reentrancy vulnerabilities

Reentrancy in DelegationOrchestrator.registerValidator(address,bytes,bytes,uint16,string,string) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#109-164): External calls: - (hasDelegated) = address(dpos).call{value: delegationValue}(abi.encodeWithSignature(delegate(address),internalValidators[i])) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#135-137) State variables written after the call(s): - externalValidatorOwners[validator] = msg.sender (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#149) - externalValidatorsByOwners[msg.sender].push(validator) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#146)
Comment on lines 109 to 164
function registerValidator(
address validator,
bytes memory proof,
bytes memory vrfKey,
uint16 commission,
string calldata description,
string calldata endpoint
) external payable override nonReentrant {
uint256 delegationValue = (2 * msg.value) / internalValidators.length;
require(
externalValidatorRegistered[validator] == 0 && internalValidatorRegistered[validator] == 0,
"Validator already registered"
);
require(msg.value >= (MIN_REGISTRATION_DELEGATION), "Sent value less than minimal delegation for registration");
require(
address(this).balance >= delegationValue * internalValidators.length,
"Insufficient funds in contract for testnet rebalance"
);
uint256 internalStake = 0;
uint256 externalStake = 0;
(internalStake, externalStake) = _calculateStakes();
require(internalStake != 0 || externalStake != 0, "Calculating current stakes failed!");
uint256 majorityStake = 2 * externalStake + msg.value + MIN_REGISTRATION_DELEGATION;
if (internalStake < majorityStake) {
// Delegate 2*tokens to our validators
for (uint8 i = 0; i < internalValidators.length; ) {
(bool hasDelegated, ) = address(dpos).call{value: delegationValue}(
abi.encodeWithSignature("delegate(address)", internalValidators[i])
);

require(hasDelegated, "Delegate call failed");
emit InternalValidatorDelegationIncreased(internalValidators[i], delegationValue);
unchecked {
++i;
}
}
}
externalValidatorsByOwners[msg.sender].push(validator);
externalValidators.push(validator);
externalValidatorRegistered[validator] = externalValidators.length;
externalValidatorOwners[validator] = msg.sender;
bytes memory input = abi.encodeWithSelector(
REGISTER_VALIDATOR_SELECTOR,
validator,
proof,
vrfKey,
commission,
description,
endpoint
);
(bool hasRegistered, ) = address(dpos).call{value: msg.value}(input);

require(hasRegistered, "Register validator call failed");

emit ExternalValidatorRegistered(validator, msg.sender, msg.value);
}

Check warning

Code scanning / Slither

Low-level calls

Low level call in DelegationOrchestrator.registerValidator(address,bytes,bytes,uint16,string,string) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#109-164): - (hasDelegated) = address(dpos).call{value: delegationValue}(abi.encodeWithSignature(delegate(address),internalValidators[i])) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#135-137) - (hasRegistered) = address(dpos).call{value: msg.value}(input) (packages/testnet-delegation/contracts/DelegationOrchestrator.sol#159)
@VargaElod23 VargaElod23 self-assigned this May 23, 2023
@VargaElod23 VargaElod23 added the deprioritized PR is required but reprioritized label May 23, 2023
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@rattrap
Copy link
Contributor

rattrap commented Aug 24, 2023

  1. Simplify internal state (we only need an array that contains our validator node addresses and a mapping between owner -> validator)
  2. No need to send tokens to the register function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprioritized PR is required but reprioritized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Delegation] New smart contract for testnet
3 participants