Skip to content

Commit

Permalink
Merge pull request #391 from lidofinance/audit/dsm-fix-comments
Browse files Browse the repository at this point in the history 
Fix COMMENTS from MixBytes audit of DepositSecurityModule
  • Loading branch information
@arwer13
arwer13 committed Jan 21, 2022
2 parents bf3c261 + 94388e6 commit 308b899
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 26 deletions.
15 changes: 14 additions & 1 deletion contracts/0.4.24/Lido.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
uint256 balance;
if (_token == ETH) {
balance = _getUnaccountedEther();
// Transfer replaced by call to prevent transfer gas amount issue
// Transfer replaced by call to prevent transfer gas amount issue
require(vault.call.value(balance)(), "RECOVER_TRANSFER_FAILED");
} else {
ERC20 token = ERC20(_token);
Expand Down Expand Up @@ -425,11 +425,19 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
NODE_OPERATORS_REGISTRY_POSITION.setStorageAddress(_r);
}

/**
* @dev Internal function to set treasury address
* @param _treasury treasury address
*/
function _setTreasury(address _treasury) internal {
require(_treasury != address(0), "SET_TREASURY_ZERO_ADDRESS");
TREASURY_POSITION.setStorageAddress(_treasury);
}

/**
* @dev Internal function to set insurance fund address
* @param _insuranceFund insurance fund address
*/
function _setInsuranceFund(address _insuranceFund) internal {
require(_insuranceFund != address(0), "SET_INSURANCE_FUND_ZERO_ADDRESS");
INSURANCE_FUND_POSITION.setStorageAddress(_insuranceFund);
Expand Down Expand Up @@ -609,6 +617,11 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
_emitTransferAfterMintingShares(treasury, toTreasury);
}

/**
* @dev Internal function to distribute reward to node operators
* @param _sharesToDistribute amount of shares to distribute
* @return actual amount of shares that was transferred to node operators as a reward
*/
function _distributeNodeOperatorsReward(uint256 _sharesToDistribute) internal returns (uint256 distributed) {
(address[] memory recipients, uint256[] memory shares) = getOperators().getRewardsDistribution(_sharesToDistribute);

Expand Down
15 changes: 3 additions & 12 deletions contracts/0.4.24/nos/NodeOperatorsRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,10 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
if (effectiveStakeTotal == 0)
return (recipients, shares);

uint256 perValidatorReward = _totalRewardShares.div(effectiveStakeTotal);
uint256 perStakeReward = _totalRewardShares.div(effectiveStakeTotal);

for (idx = 0; idx < activeCount; ++idx) {
shares[idx] = shares[idx].mul(perValidatorReward);
shares[idx] = shares[idx].mul(perStakeReward);
}

return (recipients, shares);
Expand Down Expand Up @@ -541,8 +541,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp

function _isEmptySigningKey(bytes memory _key) internal pure returns (bool) {
assert(_key.length == PUBKEY_LENGTH);
// algorithm applicability constraint
assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64);

uint256 k1;
uint256 k2;
Expand All @@ -555,7 +553,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
}

function to64(uint256 v) internal pure returns (uint64) {
assert(v <= uint256(uint64(-1)));
assert(v <= UINT64_MAX);
return uint64(v);
}

Expand All @@ -566,9 +564,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
function _storeSigningKey(uint256 _operator_id, uint256 _keyIndex, bytes memory _key, bytes memory _signature) internal {
assert(_key.length == PUBKEY_LENGTH);
assert(_signature.length == SIGNATURE_LENGTH);
// algorithm applicability constraints
assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64);
assert(0 == SIGNATURE_LENGTH % 32);

// key
uint256 offset = _signingKeyOffset(_operator_id, _keyIndex);
Expand Down Expand Up @@ -646,10 +641,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
}

function _loadSigningKey(uint256 _operator_id, uint256 _keyIndex) internal view returns (bytes memory key, bytes memory signature) {
// algorithm applicability constraints
assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64);
assert(0 == SIGNATURE_LENGTH % 32);

uint256 offset = _signingKeyOffset(_operator_id, _keyIndex);

// key
Expand Down
22 changes: 9 additions & 13 deletions contracts/0.8.9/DepositSecurityModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,8 @@ contract DepositSecurityModule {
_setMaxDeposits(_maxDepositsPerBlock);
_setMinDepositBlockDistance(_minDepositBlockDistance);
_setPauseIntentValidityPeriodBlocks(_pauseIntentValidityPeriodBlocks);

paused = false;
lastDepositBlock = 0;
}


/**
* Returns the owner address.
*/
Expand Down Expand Up @@ -149,14 +145,14 @@ contract DepositSecurityModule {


/**
* Returns `PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS` (see `pauseDeposits`).
* Returns current `pauseIntentValidityPeriodBlocks` contract parameter (see `pauseDeposits`).
*/
function getPauseIntentValidityPeriodBlocks() external view returns (uint256) {
return pauseIntentValidityPeriodBlocks;
}

/**
* Sets `PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS`. Only callable by the owner.
* Sets `pauseIntentValidityPeriodBlocks`. Only callable by the owner.
*/
function setPauseIntentValidityPeriodBlocks(uint256 newValue) external onlyOwner {
_setPauseIntentValidityPeriodBlocks(newValue);
Expand All @@ -170,14 +166,14 @@ contract DepositSecurityModule {


/**
* Returns `MAX_DEPOSITS_PER_BLOCK` (see `depositBufferedEther`).
* Returns `maxDepositsPerBlock` (see `depositBufferedEther`).
*/
function getMaxDeposits() external view returns (uint256) {
return maxDepositsPerBlock;
}

/**
* Sets `MAX_DEPOSITS_PER_BLOCK`. Only callable by the owner.
* Sets `maxDepositsPerBlock`. Only callable by the owner.
*/
function setMaxDeposits(uint256 newValue) external onlyOwner {
_setMaxDeposits(newValue);
Expand All @@ -190,14 +186,14 @@ contract DepositSecurityModule {


/**
* Returns `MIN_DEPOSIT_BLOCK_DISTANCE` (see `depositBufferedEther`).
* Returns `minDepositBlockDistance` (see `depositBufferedEther`).
*/
function getMinDepositBlockDistance() external view returns (uint256) {
return minDepositBlockDistance;
}

/**
* Sets `MIN_DEPOSIT_BLOCK_DISTANCE`. Only callable by the owner.
* Sets `minDepositBlockDistance`. Only callable by the owner.
*/
function setMinDepositBlockDistance(uint256 newValue) external onlyOwner {
_setMinDepositBlockDistance(newValue);
Expand Down Expand Up @@ -330,7 +326,7 @@ contract DepositSecurityModule {
* is a valid signature by the guardian with index guardianIndex of the data
* defined below.
*
* 2. block.number - blockNumber <= PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS
* 2. block.number - blockNumber <= pauseIntentValidityPeriodBlocks
*
* The signature, if present, must be produced for keccak256 hash of the following
* message (each component taking 32 bytes):
Expand Down Expand Up @@ -393,14 +389,14 @@ contract DepositSecurityModule {


/**
* Calls Lido.depositBufferedEther(MAX_DEPOSITS_PER_BLOCK).
* Calls Lido.depositBufferedEther(maxDepositsPerBlock).
*
* Reverts if any of the following is true:
* 1. IDepositContract.get_deposit_root() != depositRoot.
* 2. INodeOperatorsRegistry.getKeysOpIndex() != keysOpIndex.
* 3. The number of guardian signatures is less than getGuardianQuorum().
* 4. An invalid or non-guardian signature received.
* 5. block.number - getLastDepositBlock() < MIN_DEPOSIT_BLOCK_DISTANCE.
* 5. block.number - getLastDepositBlock() < minDepositBlockDistance.
* 6. blockhash(blockNumber) != blockHash.
*
* Signatures must be sorted in ascending order by index of the guardian. Each signature must
Expand Down

0 comments on commit 308b899

Please sign in to comment.