diff --git a/future-apps/payroll/README.md b/future-apps/payroll/README.md index 1647063ebf..0dfcb60c51 100644 --- a/future-apps/payroll/README.md +++ b/future-apps/payroll/README.md @@ -30,7 +30,6 @@ Set the exchange rate for an allowed token against the Payroll denomination toke Three options can be used: ``` payroll.addEmployee(address accountAddress, uint256 initialYearlyDenominationSalary, string role, uint256 startDate) -payroll.addEmployeeNow(address accountAddress, uint256 initialYearlyDenominationSalary, string role) ``` Add employee to the organization. Start date is used as the initial payment day. If it's not provided, the date of the transaction will be used. It needs `ADD_EMPLOYEE_ROLE`. diff --git a/future-apps/payroll/contracts/Payroll.sol b/future-apps/payroll/contracts/Payroll.sol index 1e1e036a18..a0eacf4ad7 100644 --- a/future-apps/payroll/contracts/Payroll.sol +++ b/future-apps/payroll/contracts/Payroll.sol @@ -176,19 +176,6 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { emit AddAllowedToken(_allowedToken); } - /** - * @notice Add employee with address `_accountAddress` to Payroll with a salary of `_initialDenominationSalary` per second - * @param _accountAddress Employee's address to receive payroll - * @param _initialDenominationSalary Employee's salary, per second in denomination token - * @param _role Employee's role - */ - function addEmployeeNow(address _accountAddress, uint256 _initialDenominationSalary, string _role) - external - authP(ADD_EMPLOYEE_ROLE, arr(_accountAddress, _initialDenominationSalary, getTimestamp())) - { - _addEmployee(_accountAddress, _initialDenominationSalary, _role, getTimestamp64()); - } - /** * @notice Add employee with address `_accountAddress` to Payroll with a salary of `_initialDenominationSalary` per second, starting on `@transformTime(_startDate)` * @param _accountAddress Employee's address to receive payroll @@ -225,18 +212,6 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { emit SetEmployeeSalary(_employeeId, _denominationSalary); } - /** - * @notice Terminate employee #`_employeeId` - * @param _employeeId Employee's identifier - */ - function terminateEmployeeNow(uint256 _employeeId) - external - authP(TERMINATE_EMPLOYEE_ROLE, arr(_employeeId)) - employeeActive(_employeeId) - { - _terminateEmployeeAt(_employeeId, getTimestamp64()); - } - /** * @notice Terminate employee #`_employeeId` on `@formatDate(_endDate)` * @param _employeeId Employee's identifier @@ -278,12 +253,12 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { /** * @notice Set token distribution for payments to an employee (the caller) - * @dev Initialization check is implicitly provided by `employeeMatches()` as new employees can + * @dev Initialization check is implicitly provided by `employeeMatches` as new employees can * only be added via `addEmployee(),` which requires initialization * @param _tokens Array with the tokens to receive, they must belong to allowed tokens for employee * @param _distribution Array, correlated to tokens, with their corresponding proportions (integers summing to 100) */ - function determineAllocation(address[] _tokens, uint256[] _distribution) external employeeMatches { + function determineAllocation(address[] _tokens, uint256[] _distribution) external employeeMatches nonReentrant { // Check arrays match require(_tokens.length == _distribution.length, ERROR_TOKEN_ALLOCATION_MISMATCH); @@ -312,11 +287,11 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { /** * @notice Request employee's payments * @dev Withdraw employee's owed payments (the caller). - * Initialization check is implicitly provided by `employeeMatches()` as new employees can only be added via `addEmployee(),` which requires initialization + * Initialization check is implicitly provided by `employeeMatches` as new employees can only be added via `addEmployee(),` which requires initialization * @param _type Payment type being requested (Payroll, Reimbursement or Bonus) * @param _requestedAmount Requested amount of the total owed to the employee for the requested payment type. Must be less or equal than total owed so far, or zero to request all owed amount */ - function payday(PaymentType _type, uint256 _requestedAmount) external employeeMatches { + function payday(PaymentType _type, uint256 _requestedAmount) external employeeMatches nonReentrant { uint256 paymentAmount; uint256 employeeId = employeeIds[msg.sender]; Employee storage employee = employees[employeeId]; @@ -344,11 +319,11 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { /** * @notice Change your employee account address to `_newAddress` * @dev Change employee's account address. Must be called by employee from their registered address. - * Initialization check is implicitly provided by `employeeMatches()` as new employees can + * Initialization check is implicitly provided by `employeeMatches` as new employees can * only be added via `addEmployee(),` which requires initialization * @param _newAddress New address to receive payments for the requesting employee */ - function changeAddressByEmployee(address _newAddress) external employeeMatches { + function changeAddressByEmployee(address _newAddress) external employeeMatches nonReentrant { // Check address is non-null require(_newAddress != address(0), ERROR_EMPLOYEE_NULL_ADDRESS); // Check address isn't already being used diff --git a/future-apps/payroll/contracts/test/mocks/MaliciousEmployee.sol b/future-apps/payroll/contracts/test/mocks/MaliciousEmployee.sol new file mode 100644 index 0000000000..266cfb90a4 --- /dev/null +++ b/future-apps/payroll/contracts/test/mocks/MaliciousEmployee.sol @@ -0,0 +1,105 @@ +pragma solidity 0.4.24; + +import "../../Payroll.sol"; +import "@aragon/os/contracts/lib/token/ERC20.sol"; +import "@aragon/os/contracts/lib/math/SafeMath.sol"; + + +contract MaliciousEmployee { + Action public action; + Payroll public payroll; + uint256 public counter; + + enum Action { Payday, ChangeAddress, SetAllocation } + + function setPayroll(Payroll _payroll) public { + payroll = _payroll; + } + + function setAction(Action _action) public { + action = _action; + } + + function payday() public { + payroll.payday(Payroll.PaymentType.Payroll, 0); + } + + function determineAllocation(address[] _tokens, uint256[] _distribution) public { + payroll.determineAllocation(_tokens, _distribution); + } + + function reenter() public { + if (counter > 0) return; + counter++; + + if (action == Action.Payday) { + payroll.payday(Payroll.PaymentType.Payroll, 0); + } + else if (action == Action.ChangeAddress) { + payroll.changeAddressByEmployee(msg.sender); + } + else if (action == Action.SetAllocation) { + address[] memory tokens = new address[](1); + tokens[0] = address(0); + uint256[] memory allocations = new uint256[](1); + allocations[0] = 100; + payroll.determineAllocation(tokens, allocations); + } + } +} + +contract MaliciousERC20 is ERC20 { + using SafeMath for uint256; + + MaliciousEmployee public employee; + uint256 private totalSupply_; + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + + constructor (MaliciousEmployee _employee, uint256 initialBalance) public { + employee = _employee; + totalSupply_ = initialBalance; + balances[msg.sender] = initialBalance; + } + + function totalSupply() public view returns (uint256) { return totalSupply_; } + + function balanceOf(address _owner) public view returns (uint256) { return balances[_owner]; } + + function allowance(address _owner, address _spender) public view returns (uint256) { return allowed[_owner][_spender]; } + + function approve(address _spender, uint256 _value) public returns (bool) { + require(allowed[msg.sender][_spender] == 0); + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + return true; + } + + function transfer(address _to, uint256 _value) public returns (bool) { + require(_value <= balances[msg.sender]); + require(_to != address(0)); + + balances[msg.sender] = balances[msg.sender].sub(_value); + balances[_to] = balances[_to].add(_value); + emit Transfer(msg.sender, _to, _value); + + employee.reenter(); + return true; + } + + function transferFrom(address _from, address _to, uint256 _value) public returns (bool) { + require(_value <= balances[_from]); + require(_value <= allowed[_from][msg.sender]); + require(_to != address(0)); + + balances[_from] = balances[_from].sub(_value); + balances[_to] = balances[_to].add(_value); + allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); + emit Transfer(_from, _to, _value); + + return true; + } +} diff --git a/future-apps/payroll/package.json b/future-apps/payroll/package.json index e8a6fa3c64..d6bdceb38e 100644 --- a/future-apps/payroll/package.json +++ b/future-apps/payroll/package.json @@ -40,7 +40,7 @@ ], "dependencies": { "@aragon/apps-finance": "3.0.0", - "@aragon/os": "4.1.0", + "@aragon/os": "4.2.0", "@aragon/ppf-contracts": "1.1.0" }, "devDependencies": { diff --git a/future-apps/payroll/test/contracts/Payroll_add_employee.test.js b/future-apps/payroll/test/contracts/Payroll_add_employee.test.js index f0517da3a0..1d0981c665 100644 --- a/future-apps/payroll/test/contracts/Payroll_add_employee.test.js +++ b/future-apps/payroll/test/contracts/Payroll_add_employee.test.js @@ -26,112 +26,6 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW)) }) - describe('addEmployeeNow', () => { - const role = 'Boss' - const salary = annualSalaryPerSecond(100000, TOKEN_DECIMALS) - - context('when it has already been initialized', function () { - beforeEach('initialize payroll app', async () => { - await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner }) - }) - - context('when the sender has permissions to add employees', () => { - const from = owner - - context('when the employee has not been added yet', () => { - let receipt, employeeId - - context('when the employee address is not the zero address', () => { - const address = employee - - beforeEach('add employee', async () => { - receipt = await payroll.addEmployeeNow(address, salary, role, { from }) - employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() - }) - - it('starts with ID 1', async () => { - assert.equal(employeeId, 1, 'first employee ID should be 1') - }) - - it('adds a new employee and emits an event', async () => { - const [address] = await payroll.getEmployee(employeeId) - assert.equal(address, employee, 'employee address does not match') - - const events = getEvents(receipt, 'AddEmployee'); - assert.equal(events.length, 1, 'number of AddEmployee events does not match') - - const event = events[0].args - assert.equal(event.employeeId, employeeId, 'employee id does not match') - assert.equal(event.role, role, 'employee role does not match') - assert.equal(event.accountAddress, employee, 'employee address does not match') - assert.equal(event.startDate.toString(), (await currentTimestamp()).toString(), 'employee start date does not match') - assert.equal(event.initialDenominationSalary.toString(), salary.toString(), 'employee salary does not match') - }) - - it('can add another employee', async () => { - const anotherRole = 'Manager' - const anotherSalary = annualSalaryPerSecond(120000, TOKEN_DECIMALS) - - const receipt = await payroll.addEmployeeNow(anotherEmployee, anotherSalary, anotherRole) - const anotherEmployeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') - - const events = getEvents(receipt, 'AddEmployee'); - assert.equal(events.length, 1, 'number of AddEmployee events does not match') - - const event = events[0].args - assert.equal(event.employeeId, anotherEmployeeId, 'employee id does not match') - assert.equal(event.role, anotherRole, 'employee role does not match') - assert.equal(event.accountAddress, anotherEmployee, 'employee address does not match') - assert.equal(event.startDate.toString(), (await currentTimestamp()).toString(), 'employee start date does not match') - assert.equal(event.initialDenominationSalary.toString(), anotherSalary.toString(), 'employee salary does not match') - - const [address, employeeSalary, bonus, reimbursements, accruedSalary, lastPayroll, endDate] = await payroll.getEmployee(anotherEmployeeId) - assert.equal(address, anotherEmployee, 'employee account does not match') - assert.equal(bonus.toString(), 0, 'employee bonus does not match') - assert.equal(reimbursements, 0, 'employee reimbursements does not match') - assert.equal(accruedSalary, 0, 'employee accrued salary does not match') - assert.equal(employeeSalary.toString(), anotherSalary.toString(), 'employee salary does not match') - assert.equal(lastPayroll.toString(), (await currentTimestamp()).toString(), 'employee last payroll does not match') - assert.equal(endDate.toString(), maxUint64(), 'employee end date does not match') - }) - }) - - context('when the employee address is not the zero address', () => { - const address = ZERO_ADDRESS - - it('reverts', async () => { - await assertRevert(payroll.addEmployeeNow(address, salary, role, { from }), 'PAYROLL_EMPLOYEE_NULL_ADDRESS') - }) - }) - }) - - context('when the employee has already been added', () => { - beforeEach('add employee', async () => { - await payroll.addEmployeeNow(employee, salary, role, { from }) - }) - - it('reverts', async () => { - await assertRevert(payroll.addEmployeeNow(employee, salary, role, { from }), 'PAYROLL_EMPLOYEE_ALREADY_EXIST') - }) - }) - }) - - context('when the sender does not have permissions to add employees', () => { - const from = anyone - - it('reverts', async () => { - await assertRevert(payroll.addEmployeeNow(employee, salary, role, { from }), 'APP_AUTH_FAILED') - }) - }) - }) - - context('when it has not been initialized yet', function () { - it('reverts', async () => { - await assertRevert(payroll.addEmployeeNow(employee, salary, role, { from: owner }), 'APP_AUTH_FAILED') - }) - }) - }) - describe('addEmployee', () => { const role = 'Boss' const salary = annualSalaryPerSecond(100000, TOKEN_DECIMALS) diff --git a/future-apps/payroll/test/contracts/Payroll_bonuses.test.js b/future-apps/payroll/test/contracts/Payroll_bonuses.test.js index 89bce466ac..c2a42728a8 100644 --- a/future-apps/payroll/test/contracts/Payroll_bonuses.test.js +++ b/future-apps/payroll/test/contracts/Payroll_bonuses.test.js @@ -43,7 +43,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 1000, 'Boss') + const receipt = await payroll.addEmployee(employee, 1000, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -103,7 +103,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the given employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await increaseTime(ONE_MONTH) }) @@ -159,7 +159,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { let employeeId, salary = 1000 beforeEach('add employee and accumulate some salary', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') await increaseTime(ONE_MONTH) @@ -263,7 +263,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesBonusesProperly(requestedAmount, bonusAmount) @@ -281,7 +281,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) context('when exchange rates are not expired', () => { @@ -318,7 +318,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesBonusesProperly(requestedAmount) @@ -336,7 +336,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesBonusesProperly(requestedAmount) @@ -354,7 +354,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesBonusesProperly(requestedAmount) @@ -372,7 +372,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) context('when exchange rates are not expired', () => { diff --git a/future-apps/payroll/test/contracts/Payroll_forwarding.test.js b/future-apps/payroll/test/contracts/Payroll_forwarding.test.js index cc51558211..8f185ad8f9 100644 --- a/future-apps/payroll/test/contracts/Payroll_forwarding.test.js +++ b/future-apps/payroll/test/contracts/Payroll_forwarding.test.js @@ -53,7 +53,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => { const sender = employee beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 100000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 100000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() }) @@ -65,7 +65,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => { context('when the employee was already terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await payroll.mockIncreaseTime(ONE_MONTH + 1) }) @@ -110,7 +110,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => { const from = employee beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 100000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 100000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() }) @@ -124,7 +124,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => { context('when the employee was already terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await payroll.mockIncreaseTime(ONE_MONTH + 1) }) diff --git a/future-apps/payroll/test/contracts/Payroll_get_employee.test.js b/future-apps/payroll/test/contracts/Payroll_get_employee.test.js index 6fb9f34fe5..c344c0c287 100644 --- a/future-apps/payroll/test/contracts/Payroll_get_employee.test.js +++ b/future-apps/payroll/test/contracts/Payroll_get_employee.test.js @@ -34,7 +34,7 @@ contract('Payroll employee getters', ([owner, employee]) => { let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 1000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 1000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() }) @@ -80,7 +80,7 @@ contract('Payroll employee getters', ([owner, employee]) => { const address = employee beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 1000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 1000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) diff --git a/future-apps/payroll/test/contracts/Payroll_modify_employee.test.js b/future-apps/payroll/test/contracts/Payroll_modify_employee.test.js index 6bfe6389ed..a5418e0779 100644 --- a/future-apps/payroll/test/contracts/Payroll_modify_employee.test.js +++ b/future-apps/payroll/test/contracts/Payroll_modify_employee.test.js @@ -43,7 +43,7 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a const previousSalary = annualSalaryPerSecond(100000, TOKEN_DECIMALS) beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, previousSalary, 'Boss') + const receipt = await payroll.addEmployee(employee, previousSalary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -109,7 +109,7 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a context('when the given employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await increaseTime(ONE_MONTH) }) @@ -158,7 +158,7 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 1000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 1000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -194,7 +194,7 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a context('when the given address belongs to another employee', () => { beforeEach('add another employee', async () => { - await payroll.addEmployeeNow(anotherEmployee, 1000, 'Boss', { from: owner }) + await payroll.addEmployee(anotherEmployee, 1000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) }) it('reverts', async () => { @@ -217,7 +217,7 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a context('when the given employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await increaseTime(ONE_MONTH) }) diff --git a/future-apps/payroll/test/contracts/Payroll_payday.test.js b/future-apps/payroll/test/contracts/Payroll_payday.test.js index c1c6cc5714..0b0b58f7b6 100644 --- a/future-apps/payroll/test/contracts/Payroll_payday.test.js +++ b/future-apps/payroll/test/contracts/Payroll_payday.test.js @@ -49,7 +49,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { const salary = 100000 beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -196,7 +196,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesPayrollProperly(requestedAmount, expectedRequestedAmount) @@ -210,7 +210,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesPayrollProperly(requestedAmount, expectedRequestedAmount) @@ -230,7 +230,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesPayrollProperly(requestedAmount, expectedRequestedAmount) @@ -244,7 +244,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) if (requestedAmount === 0 || requestedAmount === totalOwedAmount) { @@ -505,7 +505,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, {from: owner}) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), {from: owner}) }) itRevertsHandlingExpiredRates(requestedAmount, nonExpiredRatesReason, expiredRatesReason) @@ -519,7 +519,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, {from: owner}) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), {from: owner}) }) itRevertsHandlingExpiredRates(requestedAmount, nonExpiredRatesReason, expiredRatesReason) @@ -531,7 +531,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { const salary = 0 beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -591,7 +591,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { const salary = maxUint256() beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -733,7 +733,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesPayrollProperly(requestedAmount) @@ -747,7 +747,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesPayrollProperly(requestedAmount) diff --git a/future-apps/payroll/test/contracts/Payroll_reentrancy.test.js b/future-apps/payroll/test/contracts/Payroll_reentrancy.test.js new file mode 100644 index 0000000000..b210c5dd50 --- /dev/null +++ b/future-apps/payroll/test/contracts/Payroll_reentrancy.test.js @@ -0,0 +1,82 @@ +const { bigExp } = require('../helpers/numbers')(web3) +const { assertRevert } = require('@aragon/test-helpers/assertThrow') +const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3) + +const MaliciousERC20 = artifacts.require('MaliciousERC20') +const MaliciousEmployee = artifacts.require('MaliciousEmployee') + +contract('Payroll reentrancy guards', ([owner]) => { + let dao, payroll, payrollBase, finance, vault, priceFeed, maliciousToken, employee + + const NOW = 1553703809 // random fixed timestamp in seconds + const ONE_MONTH = 60 * 60 * 24 * 31 + const TWO_MONTHS = ONE_MONTH * 2 + const RATE_EXPIRATION_TIME = TWO_MONTHS + + const REENTRANCY_ACTIONS = { PAYDAY: 0, CHANGE_ADDRESS: 1, SET_ALLOCATION: 2 } + + const increaseTime = async seconds => { + await payroll.mockIncreaseTime(seconds) + await priceFeed.mockIncreaseTime(seconds) + } + + before('deploy base apps', async () => { + ({ dao, finance, vault, payrollBase } = await deployContracts(owner)) + }) + + before('allow malicious employee and token', async () => { + employee = await MaliciousEmployee.new() + + const amount = bigExp(10000, 18) + maliciousToken = await MaliciousERC20.new(employee.address, amount, { from: owner }) + await maliciousToken.approve(finance.address, amount, { from: owner }) + await finance.deposit(maliciousToken.address, amount, 'Initial deployment', { from: owner }) + }) + + beforeEach('create payroll and price feed instances', async () => { + ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW)) + await payroll.initialize(finance.address, maliciousToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner }) + await payroll.addAllowedToken(maliciousToken.address, { from: owner }) + }) + + describe('reentrancy guards', () => { + + beforeEach('add malicious employee, set tokens allocations, and accrue some salary', async () => { + await employee.setPayroll(payroll.address) + await payroll.addEmployee(employee.address, 1, 'Malicious Boss', await payroll.getTimestampPublic(), { from: owner }) + + await employee.determineAllocation([maliciousToken.address], [100]) + await increaseTime(ONE_MONTH) + }) + + describe('determineAllocation', function () { + beforeEach('set reentrancy action', async () => { + await employee.setAction(REENTRANCY_ACTIONS.SET_ALLOCATION) + }) + + it('reverts', async () => { + await assertRevert(employee.payday(), 'REENTRANCY_REENTRANT_CALL') + }) + }) + + describe('changeAddressByEmployee', function () { + beforeEach('set reentrancy action', async () => { + await employee.setAction(REENTRANCY_ACTIONS.CHANGE_ADDRESS) + }) + + it('reverts', async () => { + await assertRevert(employee.payday(), 'REENTRANCY_REENTRANT_CALL') + }) + }) + + describe('payday', function () { + beforeEach('set reentrancy action', async () => { + await employee.setAction(REENTRANCY_ACTIONS.PAYDAY) + }) + + it('reverts', async () => { + await assertRevert(employee.payday(), 'REENTRANCY_REENTRANT_CALL') + }) + }) + }) +}) diff --git a/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js b/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js index 8e9fcbe026..0674e39709 100644 --- a/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js +++ b/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js @@ -43,7 +43,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 1000, 'Boss') + const receipt = await payroll.addEmployee(employee, 1000, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -92,7 +92,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the given employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await increaseTime(ONE_MONTH) }) @@ -148,7 +148,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { let employeeId, salary = 1000 beforeEach('add employee and accumulate some salary', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') await increaseTime(ONE_MONTH) @@ -252,7 +252,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesReimbursementsProperly(requestedAmount, reimbursement) @@ -270,7 +270,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) context('when exchange rates are not expired', () => { @@ -307,7 +307,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesReimbursementsProperly(requestedAmount) @@ -325,7 +325,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesReimbursementsProperly(requestedAmount) @@ -343,7 +343,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) itHandlesReimbursementsProperly(requestedAmount) @@ -361,7 +361,7 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { context('when the employee is terminated', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) }) context('when exchange rates are not expired', () => { diff --git a/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js b/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js index 87eb3f786a..89fcce510d 100644 --- a/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js +++ b/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js @@ -30,138 +30,6 @@ contract('Payroll employees termination', ([owner, employee, anyone]) => { ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW)) }) - describe('terminateEmployeeNow', () => { - context('when it has already been initialized', function () { - beforeEach('initialize payroll app', async () => { - await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner }) - }) - - context('when the given employee id exists', () => { - let employeeId - const salary = annualSalaryPerSecond(100000, TOKEN_DECIMALS) - - beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss', { from: owner }) - employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() - }) - - context('when the sender has permissions to terminate employees', () => { - const from = owner - - context('when the employee was not terminated', () => { - beforeEach('allow denomination token', async () => { - await payroll.addAllowedToken(denominationToken.address, { from: owner }) - }) - - it('sets the end date of the employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from }) - - const endDate = (await payroll.getEmployee(employeeId))[6] - assert.equal(endDate.toString(), (await currentTimestamp()).toString(), 'employee end date does not match') - }) - - it('emits an event', async () => { - const receipt = await payroll.terminateEmployeeNow(employeeId, { from }) - - const events = getEvents(receipt, 'TerminateEmployee') - assert.equal(events.length, 1, 'number of TerminateEmployee events does not match') - - const event = events[0].args - assert.equal(event.employeeId.toString(), employeeId, 'employee id does not match') - assert.equal(event.accountAddress, employee, 'employee address does not match') - assert.equal(event.endDate.toString(), (await currentTimestamp()).toString(), 'employee end date does not match') - }) - - it('does not reset the owed salary nor the reimbursements of the employee', async () => { - const previousBalance = await denominationToken.balanceOf(employee) - await payroll.determineAllocation([denominationToken.address], [100], { from: employee }) - - // Accrue some salary and extras - await increaseTime(ONE_MONTH) - const owedSalary = salary.mul(ONE_MONTH) - const reimbursement = 1000 - await payroll.addReimbursement(employeeId, reimbursement, { from: owner }) - - // Terminate employee and travel some time in the future - await payroll.terminateEmployeeNow(employeeId, { from }) - await increaseTime(ONE_MONTH) - - // Request owed money - await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: employee }) - await payroll.payday(PAYMENT_TYPES.REIMBURSEMENT, 0, { from: employee }) - await assertRevert(payroll.getEmployee(employeeId), 'PAYROLL_EMPLOYEE_DOESNT_EXIST') - - const currentBalance = await denominationToken.balanceOf(employee) - const expectedCurrentBalance = previousBalance.plus(owedSalary).plus(reimbursement) - assert.equal(currentBalance.toString(), expectedCurrentBalance.toString(), 'current balance does not match') - }) - - it('can re-add a removed employee', async () => { - await payroll.determineAllocation([denominationToken.address], [100], { from: employee }) - await increaseTime(ONE_MONTH) - - // Terminate employee and travel some time in the future - await payroll.terminateEmployeeNow(employeeId, { from }) - await increaseTime(ONE_MONTH) - - // Request owed money - await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: employee }) - await assertRevert(payroll.getEmployee(employeeId), 'PAYROLL_EMPLOYEE_DOESNT_EXIST') - - // Add employee back - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') - const newEmployeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') - - const [address, employeeSalary, bonus, reimbursements, accruedSalary, lastPayroll, endDate] = await payroll.getEmployee(newEmployeeId) - assert.equal(address, employee, 'employee address does not match') - assert.equal(employeeSalary.toString(), salary.toString(), 'employee salary does not match') - assert.equal(lastPayroll.toString(), (await currentTimestamp()).toString(), 'employee last payroll date does not match') - assert.equal(bonus.toString(), 0, 'employee bonus does not match') - assert.equal(reimbursements.toString(), 0, 'employee reimbursements does not match') - assert.equal(accruedSalary.toString(), 0, 'employee accrued salary does not match') - assert.equal(endDate.toString(), maxUint64(), 'employee end date does not match') - }) - }) - - context('when the employee was already terminated', () => { - beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from }) - await increaseTime(ONE_MONTH + 1) - }) - - it('reverts', async () => { - await assertRevert(payroll.terminateEmployeeNow(employeeId, { from }), 'PAYROLL_NON_ACTIVE_EMPLOYEE') - }) - }) - }) - - context('when the sender does not have permissions to terminate employees', () => { - const from = anyone - - it('reverts', async () => { - await assertRevert(payroll.terminateEmployeeNow(employeeId, { from }), 'APP_AUTH_FAILED') - }) - }) - }) - - context('when the given employee id does not exist', () => { - const employeeId = 0 - - it('reverts', async () => { - await assertRevert(payroll.terminateEmployeeNow(employeeId, { from: owner }), 'PAYROLL_NON_ACTIVE_EMPLOYEE') - }) - }) - }) - - context('when it has not been initialized yet', function () { - const employeeId = 0 - - it('reverts', async () => { - await assertRevert(payroll.terminateEmployeeNow(employeeId, { from: owner }), 'APP_AUTH_FAILED') - }) - }) - }) - describe('terminateEmployee', () => { context('when it has already been initialized', function () { beforeEach('initialize payroll app', async () => { @@ -173,7 +41,7 @@ contract('Payroll employees termination', ([owner, employee, anyone]) => { const salary = annualSalaryPerSecond(100000, TOKEN_DECIMALS) beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString() }) @@ -248,7 +116,7 @@ contract('Payroll employees termination', ([owner, employee, anyone]) => { await assertRevert(payroll.getEmployee(employeeId), 'PAYROLL_EMPLOYEE_DOESNT_EXIST') // Add employee back - const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss') + const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic()) const newEmployeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') const [address, employeeSalary, bonus, reimbursements, accruedSalary, lastPayroll, date] = await payroll.getEmployee(newEmployeeId) diff --git a/future-apps/payroll/test/contracts/Payroll_token_allocations.test.js b/future-apps/payroll/test/contracts/Payroll_token_allocations.test.js index 80832c40a7..906b7104a8 100644 --- a/future-apps/payroll/test/contracts/Payroll_token_allocations.test.js +++ b/future-apps/payroll/test/contracts/Payroll_token_allocations.test.js @@ -47,7 +47,7 @@ contract('Payroll token allocations', ([owner, employee, anyone]) => { let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 100000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 100000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -175,7 +175,7 @@ contract('Payroll token allocations', ([owner, employee, anyone]) => { context('when the employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await payroll.mockIncreaseTime(ONE_MONTH) }) @@ -209,7 +209,7 @@ contract('Payroll token allocations', ([owner, employee, anyone]) => { let employeeId beforeEach('add employee', async () => { - const receipt = await payroll.addEmployeeNow(employee, 100000, 'Boss', { from: owner }) + const receipt = await payroll.addEmployee(employee, 100000, 'Boss', await payroll.getTimestampPublic(), { from: owner }) employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId') }) @@ -289,7 +289,7 @@ contract('Payroll token allocations', ([owner, employee, anyone]) => { context('when the employee is not active', () => { beforeEach('terminate employee', async () => { - await payroll.terminateEmployeeNow(employeeId, { from: owner }) + await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner }) await payroll.mockIncreaseTime(ONE_MONTH) })