Skip to content

Commit

Permalink
Fix audit report (#194)
Browse files Browse the repository at this point in the history 
* fix audit report

* fix oz contract import

* fix core test failing

* update core package version

* fix eslint

* fix jest cli version

* fix dependency

* revert yarn.lock
  • Loading branch information
@leric7
leric7 authored Jan 31, 2023
1 parent 8440e93 commit 7f950f5
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 59 deletions.
72 changes: 55 additions & 17 deletions packages/core/contracts/Escrow.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,28 @@ pragma solidity >=0.6.2;

import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
import '@openzeppelin/contracts/security/ReentrancyGuard.sol';

import './interfaces/IRewardPool.sol';
import './interfaces/IEscrow.sol';
import './utils/SafeMath.sol';

contract Escrow is IEscrow {
contract Escrow is IEscrow, ReentrancyGuard {
using SafeMath for uint256;

bytes4 private constant FUNC_SELECTOR_BALANCE_OF =
bytes4(keccak256('balanceOf(address)'));
bytes4 private constant FUNC_SELECTOR_TRANSFER =
bytes4(keccak256('transfer(address,uint256)'));

string constant ERROR_ZERO_ADDRESS = 'Escrow: zero address';

event TrustedHandlerAdded(address _handler);
event IntermediateStorage(string _url, string _hash);
event Pending(string manifest, string hash);
event BulkTransfer(uint256 indexed _txId, uint256 _bulkCount);
event Cancelled();
event Completed();

EscrowStatuses public override status;

Expand All @@ -30,7 +36,7 @@ contract Escrow is IEscrow {

uint256 public reputationOracleStake;
uint256 public recordingOracleStake;
uint256 private constant BULK_MAX_VALUE = 1000000000 * (10 ** 18);
uint256 private constant BULK_MAX_VALUE = 1e9 * (10 ** 18);
uint32 private constant BULK_MAX_COUNT = 100;

address public token;
Expand All @@ -55,6 +61,9 @@ contract Escrow is IEscrow {
uint256 _duration,
address[] memory _handlers
) {
require(_token != address(0), ERROR_ZERO_ADDRESS);
require(_canceler != address(0), ERROR_ZERO_ADDRESS);

token = _token;
status = EscrowStatuses.Launched;
duration = _duration.add(block.timestamp); // solhint-disable-line not-rely-on-time
Expand All @@ -81,7 +90,9 @@ contract Escrow is IEscrow {
'Address calling cannot add trusted handlers'
);
for (uint256 i = 0; i < _handlers.length; i++) {
require(_handlers[i] != address(0), ERROR_ZERO_ADDRESS);
areTrustedHandlers[_handlers[i]] = true;
emit TrustedHandlerAdded(_handlers[i]);
}
}

Expand All @@ -96,7 +107,7 @@ contract Escrow is IEscrow {
string memory _url,
string memory _hash,
uint256 _solutionsRequested
) public trusted notExpired {
) external trusted notExpired {
require(
_reputationOracle != address(0),
'Invalid or missing token spender'
Expand All @@ -107,7 +118,7 @@ contract Escrow is IEscrow {
);
require(_solutionsRequested > 0, 'Invalid or missing solutions');
uint256 totalStake = _reputationOracleStake.add(_recordingOracleStake);
require(totalStake >= 0 && totalStake <= 100, 'Stake out of bounds');
require(totalStake <= 100, 'Stake out of bounds');
require(
status == EscrowStatuses.Launched,
'Escrow not in Launched status state'
Expand All @@ -128,7 +139,7 @@ contract Escrow is IEscrow {
emit Pending(manifestUrl, manifestHash);
}

function abort() public trusted notComplete notPaid {
function abort() external trusted notComplete notPaid {
if (getBalance() != 0) {
cancel();
}
Expand All @@ -141,31 +152,35 @@ contract Escrow is IEscrow {
notBroke
notComplete
notPaid
nonReentrant
returns (bool)
{
_safeTransfer(canceler, getBalance());
status = EscrowStatuses.Cancelled;
emit Cancelled();
return true;
}

function complete() public notExpired {
function complete() external notExpired {
require(
msg.sender == reputationOracle || areTrustedHandlers[msg.sender],
areTrustedHandlers[msg.sender],
'Address calling is not trusted'
);
require(status == EscrowStatuses.Paid, 'Escrow not in Paid state');
status = EscrowStatuses.Complete;
emit Completed();
}

function storeResults(
string memory _url,
string memory _hash
) public trusted notExpired {
) external trusted notExpired {
require(
status == EscrowStatuses.Pending ||
status == EscrowStatuses.Partial,
'Escrow not in Pending or Partial status state'
);
_storeResult(_url, _hash);
emit IntermediateStorage(_url, _hash);
}

Expand All @@ -175,39 +190,53 @@ contract Escrow is IEscrow {
string memory _url,
string memory _hash,
uint256 _txId
) public trusted notBroke notLaunched notPaid notExpired returns (bool) {
)
external
trusted
notBroke
notLaunched
notPaid
notExpired
nonReentrant
returns (bool)
{
require(
_recipients.length == _amounts.length,
"Amount of recipients and values don't match"
);
require(_recipients.length < BULK_MAX_COUNT, 'Too many recipients');
require(
status != EscrowStatuses.Complete &&
status != EscrowStatuses.Cancelled,
'Invalid status'
);

uint256 balance = getBalance();
bulkPaid = false;
uint256 aggregatedBulkAmount = 0;
for (uint256 i; i < _amounts.length; i++) {
aggregatedBulkAmount += _amounts[i];
aggregatedBulkAmount = aggregatedBulkAmount.add(_amounts[i]);
}
require(aggregatedBulkAmount < BULK_MAX_VALUE, 'Bulk value too high');

if (balance < aggregatedBulkAmount) {
return bulkPaid;
}

bool writeOnchain = bytes(_hash).length != 0 || bytes(_url).length != 0;
if (writeOnchain) {
// Be sure they are both zero if one of them is
finalResultsUrl = _url;
finalResultsHash = _hash;
}
_storeResult(_url, _hash);

(
uint256 reputationOracleFee,
uint256 recordingOracleFee
) = finalizePayouts(_amounts);

for (uint256 i = 0; i < _recipients.length; ++i) {
_safeTransfer(_recipients[i], finalAmounts[i]);
uint256 amount = finalAmounts[i];
if (amount == 0) {
continue;
}
finalAmounts[i] = 0;
_safeTransfer(_recipients[i], amount);
}

delete finalAmounts;
Expand Down Expand Up @@ -268,6 +297,15 @@ contract Escrow is IEscrow {
SafeERC20.safeTransfer(IERC20(token), to, value);
}

function _storeResult(string memory _url, string memory _hash) internal {
bool writeOnchain = bytes(_hash).length != 0 || bytes(_url).length != 0;
if (writeOnchain) {
// Be sure both of them are not zero
finalResultsUrl = _url;
finalResultsHash = _hash;
}
}

modifier trusted() {
require(areTrustedHandlers[msg.sender], 'Address calling not trusted');
_;
Expand Down
33 changes: 19 additions & 14 deletions packages/core/contracts/HMToken.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract HMToken is HMTokenInterface, Ownable {
function transfer(
address _to,
uint256 _value
) public override returns (bool success) {
) external override returns (bool success) {
success = transferQuiet(_to, _value);
require(success, "Transfer didn't succeed");
return success;
Expand All @@ -62,7 +62,11 @@ contract HMToken is HMTokenInterface, Ownable {
address _spender,
address _to,
uint256 _value
) public override returns (bool success) {
) external override returns (bool success) {
require(
_spender != address(0),
"Can't send tokens to uninitialized address"
);
uint256 _allowance = allowed[_spender][msg.sender];
require(_allowance >= _value, 'Spender allowance too low');
require(
Expand All @@ -89,14 +93,14 @@ contract HMToken is HMTokenInterface, Ownable {

function balanceOf(
address _owner
) public view override returns (uint256 balance) {
) external view override returns (uint256 balance) {
return balances[_owner];
}

function approve(
address _spender,
uint256 _value
) public override returns (bool success) {
) external override returns (bool success) {
require(
_spender != address(0),
'Token spender is an uninitialized address'
Expand All @@ -118,8 +122,7 @@ contract HMToken is HMTokenInterface, Ownable {

uint256 _oldValue = allowed[msg.sender][_spender];
if (
_oldValue.add(_delta) < _oldValue ||
_oldValue.add(_delta) >= MAX_UINT256
_oldValue + _delta < _oldValue || _oldValue + _delta == MAX_UINT256
) {
// Truncate upon overflow.
allowed[msg.sender][_spender] = MAX_UINT256.sub(1);
Expand All @@ -135,7 +138,7 @@ contract HMToken is HMTokenInterface, Ownable {
function decreaseApproval(
address _spender,
uint256 _delta
) public returns (bool success) {
) external returns (bool success) {
require(
_spender != address(0),
'Token spender is an uninitialized address'
Expand All @@ -157,15 +160,15 @@ contract HMToken is HMTokenInterface, Ownable {
function allowance(
address _owner,
address _spender
) public view override returns (uint256 remaining) {
) external view override returns (uint256 remaining) {
return allowed[_owner][_spender];
}

function transferBulk(
address[] memory _tos,
uint256[] memory _values,
uint256 _txId
) public override returns (uint256 _bulkCount) {
) external override returns (uint256 _bulkCount) {
require(
_tos.length == _values.length,
"Amount of recipients and values don't match"
Expand All @@ -189,11 +192,11 @@ contract HMToken is HMTokenInterface, Ownable {
return _bulkCount;
}

function approveBulk(
function increaseApprovalBulk(
address[] memory _spenders,
uint256[] memory _values,
uint256 _txId
) public returns (uint256 _bulkCount) {
) external returns (uint256 _bulkCount) {
require(
_spenders.length == _values.length,
"Amount of spenders and values don't match"
Expand Down Expand Up @@ -222,9 +225,11 @@ contract HMToken is HMTokenInterface, Ownable {
address _to,
uint256 _value
) internal returns (bool success) {
if (_to == address(0)) return false; // Preclude burning tokens to uninitialized address.
if (_to == address(this)) return false; // Preclude sending tokens to the contract.
if (balances[msg.sender] < _value) return false;
if (
_to == address(0) ||
_to == address(this) ||
balances[msg.sender] < _value
) return false; // Preclude burning tokens to uninitialized address, or sending tokens to the contract.

balances[msg.sender] = balances[msg.sender] - _value;
balances[_to] = balances[_to].add(_value);
Expand Down
6 changes: 3 additions & 3 deletions packages/core/contracts/RewardPool.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

pragma solidity >=0.6.2;

import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
import '@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol';
import '@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol';
import '@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol';
import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol';

Expand Down Expand Up @@ -118,7 +118,7 @@ contract RewardPool is IRewardPool, OwnableUpgradeable, UUPSUpgradeable {
}

function _safeTransfer(address to, uint256 value) internal {
SafeERC20.safeTransfer(IERC20(token), to, value);
SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), to, value);
}

modifier onlyStaking() {
Expand Down
13 changes: 9 additions & 4 deletions packages/core/contracts/Staking.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

pragma solidity >=0.6.2;

import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
import '@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol';
import '@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol';
import '@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol';
import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol';

Expand Down Expand Up @@ -548,15 +548,20 @@ contract Staking is IStaking, OwnableUpgradeable, UUPSUpgradeable {
}

function _safeTransfer(address to, uint256 value) internal {
SafeERC20.safeTransfer(IERC20(token), to, value);
SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), to, value);
}

function _safeTransferFrom(
address from,
address to,
uint256 value
) internal {
SafeERC20.safeTransferFrom(IERC20(token), from, to, value);
SafeERC20Upgradeable.safeTransferFrom(
IERC20Upgradeable(token),
from,
to,
value
);
}

modifier onlyStaker(address _staker) {
Expand Down
8 changes: 5 additions & 3 deletions packages/core/test/EscrowFactory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ describe('EscrowFactory', function () {
.connect(operator)
.createEscrow(token.address, trustedHandlers)
).wait();
const event = result.events?.[0].args;
const event = result.events?.find(({ topics }) =>
topics.includes(ethers.utils.id('Launched(address,address)'))
)?.args;

return event;
}
Expand Down Expand Up @@ -85,7 +87,7 @@ describe('EscrowFactory', function () {
await expect(
escrowFactory
.connect(operator)
.createEscrow(token.address, [ethers.constants.AddressZero])
.createEscrow(token.address, [await reputationOracle.getAddress()])
).to.be.revertedWith('Needs to stake HMT tokens to create an escrow.');
});

Expand Down Expand Up @@ -141,7 +143,7 @@ describe('EscrowFactory', function () {
await expect(
escrowFactory
.connect(operator)
.createEscrow(token.address, [ethers.constants.AddressZero])
.createEscrow(token.address, [await reputationOracle.getAddress()])
).to.be.revertedWith('Needs to stake HMT tokens to create an escrow.');
});

Expand Down
Loading

0 comments on commit 7f950f5

Please sign in to comment.