Skip to content

Commit

Permalink
Payroll: Add reentracy guards
Browse files Browse the repository at this point in the history
  • Loading branch information
@facuspagnuolo
facuspagnuolo committed Apr 16, 2019
1 parent 1d4edab commit db60d64
Show file tree
Hide file tree
Showing 14 changed files with 240 additions and 317 deletions.
1 change: 0 additions & 1 deletion future-apps/payroll/README.md
View file
Expand Up @@ -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`.

Expand Down
37 changes: 6 additions & 31 deletions future-apps/payroll/contracts/Payroll.sol
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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
Expand Down
105 changes: 105 additions & 0 deletions future-apps/payroll/contracts/test/mocks/MaliciousEmployee.sol
View file
@@ -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;
}
}
2 changes: 1 addition & 1 deletion future-apps/payroll/package.json
View file
Expand Up @@ -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": {
Expand Down
106 changes: 0 additions & 106 deletions future-apps/payroll/test/contracts/Payroll_add_employee.test.js
View file
Expand Up @@ -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)
Expand Down
18 changes: 9 additions & 9 deletions future-apps/payroll/test/contracts/Payroll_bonuses.test.js
View file
Expand Up @@ -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')
})

Expand Down Expand Up @@ -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)
})

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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', () => {
Expand Down

0 comments on commit db60d64

Please sign in to comment.