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

Resolve Certik v3 Reports #437

Merged
merged 4 commits into from
May 5, 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
38 changes: 28 additions & 10 deletions packages/core/contracts/Escrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,9 @@ contract Escrow is IEscrow, ReentrancyGuard {
return 0;
}

function addTrustedHandlers(address[] memory _handlers) public override {
require(
areTrustedHandlers[msg.sender],
'Address calling cannot add trusted handlers'
);
function addTrustedHandlers(
address[] memory _handlers
) public override trusted {
for (uint256 i = 0; i < _handlers.length; i++) {
require(_handlers[i] != address(0), ERROR_ZERO_ADDRESS);
areTrustedHandlers[_handlers[i]] = true;
Expand Down Expand Up @@ -182,8 +180,21 @@ contract Escrow is IEscrow, ReentrancyGuard {
}

/**
* @dev Bulk payout workers
* Should fail if any of the transaction is failing.
* @dev Performs bulk payout to multiple workers
Copy link
Contributor

Choose a reason for hiding this comment

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

This should stay @notice : Explain to an end user what this doe
@dev is the tag to : Explain to a developer any extra details
@notice : Performs bulk payout to multiple workers
@dev Escrow needs to be completed ...

I think certik wanted to have a clear comments :

  • solc --devdoc --pretty-json Escrow.sol will return details with comments.

to check [https://coinsbench.com/natspec-the-right-way-to-comment-ethereum-smart-contracts-6762082252d4]

* Escrow needs to be complted / cancelled, so that it can be paid out.
leric7 marked this conversation as resolved.
Show resolved Hide resolved
* Every recipient is paid with the amount after reputation and recording oracle fees taken out.
* If the amount is less than the fee, the recipient is not paid.
* If the fee is zero, reputation, and recording oracle are not paid.
* Payout will fail if any of the transaction fails.
* If the escrow is fully paid out, meaning that the balance of the escrow is 0, it'll set as Paid.
* If the escrow is partially paid out, meaning that the escrow still has remaining balance, it'll set as Partial.
* This contract is only callable if the contract is not broke, not launched, not paid, not expired, by trusted parties.
*
* @param _recipients Array of recipients
* @param _amounts Array of amounts to be paid to each recipient.
* @param _url URL storing results as transaction details
* @param _hash Hash of the results
* @param _txId Transaction ID
*/
leric7 marked this conversation as resolved.
Show resolved Hide resolved
function bulkPayOut(
address[] memory _recipients,
Expand Down Expand Up @@ -215,6 +226,7 @@ contract Escrow is IEscrow, ReentrancyGuard {
uint256 balance = getBalance();
uint256 aggregatedBulkAmount = 0;
for (uint256 i; i < _amounts.length; i++) {
require(_amounts[i] > 0, 'Amount should be greater than zero');
aggregatedBulkAmount = aggregatedBulkAmount.add(_amounts[i]);
}
require(aggregatedBulkAmount < BULK_MAX_VALUE, 'Bulk value too high');
Expand All @@ -229,11 +241,17 @@ contract Escrow is IEscrow, ReentrancyGuard {
) = finalizePayouts(_amounts);
leric7 marked this conversation as resolved.
Show resolved Hide resolved

for (uint256 i = 0; i < _recipients.length; ++i) {
_safeTransfer(_recipients[i], finalAmounts[i]);
if (finalAmounts[i] > 0) {
portuu3 marked this conversation as resolved.
Show resolved Hide resolved
_safeTransfer(_recipients[i], finalAmounts[i]);
}
}

_safeTransfer(reputationOracle, reputationOracleFee);
_safeTransfer(recordingOracle, recordingOracleFee);
if (reputationOracleFee > 0) {
_safeTransfer(reputationOracle, reputationOracleFee);
}
if (recordingOracleFee > 0) {
_safeTransfer(recordingOracle, recordingOracleFee);
}

balance = getBalance();

Expand Down
20 changes: 10 additions & 10 deletions packages/core/contracts/HMToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ contract HMToken is HMTokenInterface, Ownable {

if (_allowance != MAX_UINT256) {
// Special case to approve unlimited transfers
allowed[_spender][msg.sender] = allowed[_spender][msg.sender].sub(
_value
);
allowed[_spender][msg.sender] =
allowed[_spender][msg.sender] -
_value;
}

emit Transfer(_spender, _to, _value);
Expand Down Expand Up @@ -125,11 +125,11 @@ contract HMToken is HMTokenInterface, Ownable {
_oldValue + _delta < _oldValue || _oldValue + _delta == MAX_UINT256
) {
// Truncate upon overflow.
allowed[msg.sender][_spender] = MAX_UINT256.sub(1);
allowed[msg.sender][_spender] = MAX_UINT256 - 1;
} else {
allowed[msg.sender][_spender] = allowed[msg.sender][_spender].add(
_delta
);
allowed[msg.sender][_spender] =
allowed[msg.sender][_spender] +
_delta;
}
emit Approval(msg.sender, _spender, allowed[msg.sender][_spender]);
return true;
Expand All @@ -149,9 +149,9 @@ contract HMToken is HMTokenInterface, Ownable {
// Truncate upon overflow.
allowed[msg.sender][_spender] = 0;
} else {
allowed[msg.sender][_spender] = allowed[msg.sender][_spender].sub(
_delta
);
allowed[msg.sender][_spender] =
allowed[msg.sender][_spender] -
_delta;
}
emit Approval(msg.sender, _spender, allowed[msg.sender][_spender]);
return true;
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/Escrow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('Escrow', function () {
escrow
.connect(externalAddress)
.addTrustedHandlers([await reputationOracle.getAddress()])
).to.be.revertedWith('Address calling cannot add trusted handlers');
).to.be.revertedWith('Address calling not trusted');
});
});

Expand Down