# That Planning Suite internal security audits

## Intro

I reviewed and audited both Rewards and AddressBook apps contracts and projects code and here are the results.

The audited code can be found on That Planning Suite repo.
I used the latest versions available on dev branch for each audit.

The git commit used for this review was: [db3e49e](https://github.com/AutarkLabs/planning-suite/tree/db3e49e8b56503d2f35ea960a9c28dc1cd6d81a7)

The aragon framework, specially ACL system, provides an extra security layer, so I found the contracts are basically safe from external malicious actors.

The overall code quality is high and the tests have 100% coverage, but this does not mean that every possible contract interaction is covered and is not a definitive metric about the contract security.

I tried to order the assessment and recommendations in order of importance.

I couldn't find critical or severe issues during the reviews.

## Rewards

### Medium/Low impact

#### Benign reentrancy in `Rewards.claimReward` and `Rewards.newReward`

- [x] Possible reentrancy vulnerabilities at lines 58-77 and 139-184, consider rewriting the logic of this function, anyway the reentrancy attack would be mostly harmless
✅ Fixed!

Recommendation: Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2

#### MiniMeToken uninitialized state variables

- [ ] Even this seems a bit out of scope, Slither tool points to a High impact finding on MiniMeToken.balance, about Uninitialized state variables, with a High confidence fo the vulnerabily.
⚠️ Not fixed! (Requires upstream changes)

Recommendation: Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero.

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-state-variables

### Warnings/Recommendations

- [x] Consider using `ethlint` instead of deprecated `solium` module
✅ Fixed!

#### Compilation

- [x] There exist several warnings about the dependencies using deprecated syntax, the general recommendation would be to fix these warnings upstream, as it could bring unexpected issues on the future.
⚠️ Not fixed! (Requires upstream changes)

- MiniMeToken.sol
- Missing constructor syntax (lines 37, 123)
- Use of deprecated `var` keyword (lines 195, 209)
- Events without `emit` keyword (lines 213, 245, 373, 392, 408, 509, 516)
- IACL.sol
- Interface function should be declared external (line 13)
- IEVMScriptRegistry
- Interface function should be declared external (line 24)
- VaultRecoverable
- Unused function param (49)

#### Recommendations

- [x] Pragma version is not locked, this allows unsupported compiler versions
✅ Fixed!

- [ ] Pragma version is too old, the recommendation is to use Solidity 0.4.25 or 0.5.3. Consider using the latest version of Solidity for testing the compilation, and a trusted version for deploying.
⚠️ Not fixed! (Requires upstream changes)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity

- [x] There is a unused import of `Migrations.sol` at line 5, this should be removed
✅ Fixed!

- [x] Consider pre-compute of `keccak256` values to save gas (line 21)
✅ Fixed!

- [x] Tight variable packing: Consider sorting members of struct at lines 23-36 to save some gas
Reference: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html
✅ Fixed!

- [ ] Event params utility is not clear, consider document the utility or removing the params
🚨❓ Pending! (Aragon app uses all params in the event, instead of just the id)

- [ ] Consider using SafeMath library instead of performing manual math calculations, to avoid eventual overflows or underflows, and divisions by zero (line 191)
🚨❓ Pending! (Is it really needed?)

- [ ] Consider using `SafeERC20` and/or `ERC20` as Vault contract does for safety on tokens management
🚨❓ Pending! (Is it really needed?)

- [x] Type contract `MiniMeToken` is not implicitly convertible to expected type address, cast it to address
✅ Fixed!

- [ ] The contract has missing documentation, consider using NatSpec to document every function and member (state, structures and contract members), this will help future devs and readers to understand what is going on with the code
🚨❓ Pending! (Missing parts, comment team improvements on radspec)

- [x] There are some spacing inconsistencies, consider linting the contract correctly:
✅ Fixed!
- Replace `returns()` by `returns ()`
- Remove or add a space at `ADD_REWARD_ROLE`, `initialize()`, `calculateMeritReward internal`

- [ ] There seems to be a pending solution to be applied at line 6 (it mentions about ocurrences issue), this should be applied before a proper audit
🚨❓ Pending! (Ask team about this)

- [x] There exists some dead code lines at lines 206, 207, consider removing them
❓ Partially fixed (Remove lines but the issue exists)

- [x] The linter was disable at line 62, consider fixing the linting warning instead
✅ Fixed!

- [x] There is duplicated logic (lines 191->210, 190->198, 189->205) consider creating intermediate functions or modifiers to reuse the code instead
✅ Fixed!

- [x] Some `storage` keywords can be replaced by `memory` at lines 59, 99, 159 if the state is not going to be modified
😎 False positive

- [x] Consider moving `required` reasons strings to constants instead
✅ Fixed!

- [x] Some `block` members are being used, consider clarify if they are not gameable by miners transaction order, and if they are safe for block timing delays, replace them otherwise (lines 60, 62)
✅ Fixed!

- [x] Consider using interface type instead of the address for type safety (for Vault and every MiniMeToken instantiations)
✅ Fixed! (RewardToken not possible because it also accepts ETH rewards)

- [ ] Reward.rewardToken could be initialized to any ethereum address
❓ Fixed partially, still needs trust in the token (pending warn comment on this)

- `_vault` should be casted to address on line 92 (because `isContract`)
✅ Fixed in commit

- [ ] Function `claimReward` lacks ACL control, is not clear if that is on purpose, but anyway, would be a good solution to guard with ACL control and then assign that ROLE to `ANY_ADDRESS` to give more flexibility
🚨❓ Pending (Needs discussion)

- [ ] Some uses of strings could be quite expensive, consider using fixed byte arrays instead of strings if possible
🚨❓ Pending (Needs discussion)

- [x] Logic for `calculateMeritReward` and `calculateDividendReward` could be a bit more clean to understand, consider rewritting them or merging into a single function
✅ Fixed

- [x] Block comments have unconsistent format
✅ Fixed

- [ ] `totalClaimsEach` naming is a bit confusing, consider documenting it or replacing by a better name to make it more understandable
🚨❓ Pending (What is this actually?)

- [x] `calculateMeritReward` could be initialized on declaration (lines 195, 196)
✅ Fixed

- [ ] Data location should be `memory` for return parameter on lines 112, 172
⚠️ Not applicable - not really an issue until solidity 0.5 - lines wrong

- [ ] Data location should be `memory` or `storage` at lines 218, 226
⚠️ Not applicable - not really an issue until solidity 0.5 - lines wrong

- [ ] Is not clear if `vault` should be public on line 114
⚠️❓ Needs discussion - Not applicable

- [x] `isContract` use has its trade-offs, clarify if that is actually needed at lines: 49, 151, 153. Replace it otherwise
✅ Fixed

- [ ] Only `getTotalAmountClaimed` uses `isInitialized`, should the rest of functions also use this modifier?
🚨❓ Pending (Needs discussion)

- [x] Consider adding extra `ethlint` config/plugins to track eventual security issues
✅ Fixed (ethlint has not additional plugins right now)

- [ ] There is no circuit breaker available like `Pausable` to halt the contract if anything goes wrong, consider if that would be needed eventually, also there is no rate limiting or maximum usage limits set to minimize the amount of money that could be put at risk
🚨❓ Pending (Needs discussion)

- [ ] There is not a clear upgrade path for bugfixes and improvements, like rollout phases
🚨❓ Pending (Needs discussion)

- [ ] Functions params are not using a consistent standard casing (solidity team suggests using mix-case for these), at functions `getReward`, `calculateDividendReward`, `calculateMeritReward`
🚨❓ Pending (Needs discussion)

- [ ] Avoid using magic numbers (line 156), is not clear why `41` is used for max "occurances", and this seems a pretty big number for this scenario, that could make the recurrent function too expensive
🚨❓Partially solved, by moving to a constant (Needs discussion)

- [x] Consider using `rewardId` instead of the full `Reward` instance on `calculateDividendReward` and `calculateMeritReward`
✅ Fixed - not applicable after refactor

- [ ] There is not a single `assert` used for formal verification by checking invariants, consider adding some assertions to the contract
🚨❓ Pending (Needs discussion)

### Tests warnings

- [ ] Tests cases are using unreal values, consider using real ones instead
🚨 Pending

- [ ] Internal functions should be tested with dedicated tests written in Solidity language: `calculateDividendReward` , `calculateMeritReward`
🚨❓ Pending (Is this really needed?)

- [ ] Tests should be able to pass individually without depending on previous test, with the Mocha keyword `.only`. This is not happenning for the following test cases:
🚨 Pending
- Creates a merit reward
- Gets information on the dividend reward
- Gets information on the merit reward
- Receives rewards dividends
- Receives rewards merit
- Gets total rewards amount claimed
- Gets total claims made
- Creates a merit reward that started in the past
- Can read rewards array length
- Creates a ETH reward
- Reverts if vault contains insufficient reward tokens

- [ ] Consider linting the test file to minimize chance of issues and improve readability (this affects unconsistent spacing, unused variables, constants declared with let)
🚨❓ Fixed partially, still needs to check let-const

- [x] Consider avoiding imports from unpublished node modules (lines 1-7, 11, 12, 13, 14)
✅ Fixed

- [x] Consider removing unused imports (line 12)
✅ Fixed

- [x] Consider removing unused variables (lines 16, 22, 131)
✅ Fixed

- [x] Consider fixing spelling typos (lines 191, 199, 338, 354)
✅ Fixed

- [x] Consider using standard and consistend format for addresses (lines 16, 17)
✅ Fixed

### Coverage warnings

- [x] Solcover config `.solcover.js` should be optimized
✅ Fixed

- [ ] Compilation of contract for coverage happens three times
⚠️🚨❓ Not applicable (solidity-coverage issue, pending link their info) - Consider using 0x devtools for coverage

### Gas report warnings

- [ ] The gas table is empty, consider optimizing tests for better gas metrics
🚨❓ Pending discussion - Consider using 0x devtools for gas metrics

### Project warnings and recommendations

- [x] Files in migrations folder should be removed, since they are preventing the contract to run correctly in isolation (truffle out of gas error)
✅ Fixed

- [x] Spoof contract is redundant, and not needed at all
✅ Fixed

___

## AddressBook

The address book contract is a simple registry of names mapped to address.

### Contract warnings / recommendations

As a suggestion, consider using the library for solidity CRUD registries developed by Rob Hitchens from Solidified.io, since he seemed to put lot of effort optimizing the gas consumptions for these kind of structures: https://medium.com/robhitchens/solidity-crud-part-1-824ffa69509a

The `nameUsed` boolean mapping for entries seems and antipattern
Some issues could happen if an user tries to create entries pointing to 0x0 address.

All functions in the contract should be declared external and `onlyInit`

The `keccak256` encodings could be computed externally to sav some gas

The `entryType` should values should be taken from an `enum` to more consistent with the current UI for that

Consider hardcoding the `require` error reasons strings into constants to make the code more readable or reusable

The `Entry storage entry` at `getEntry` could be replaced by `memory` since is not intended to modify the state

Pragma and contract depedencies suffer the same warnings as Rewards (except Vault and MiniMeToken not used here)

The contract could be formally verified by adding some asserts to check invariants

The contract is lacking a function to get the registry length

Consider using an intermediate function to check if an entry exists in the contract

The comment and commented at lines 78-81 could be deleted.

Checks like entries[address].entryAddress seem a redundant terminology, consider replacing the naming or logic to one that is less redundant / confusing

Basically most issues would be solved if the previous mentioned registry library is used instead

### Project recommendations

Generally the same as Rewards related to tests, migration files, ddependencies and imports, coverage and gas metrics configurations