# That Planning Suite internal security audits (II)

## Intro

I reviewed and audited Projects apps contract code and configuration and here are the results

The audited code can be found on That Planning Suite repo, it has the latest StandarBounties integration

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

I tried to sort the findings in order of importance

Some edge cases and attack vectors were skipped due to time constraints and the contract size and complexity, a general recommendation would be create additional tests to cover these potential spots

## Projects contract potential issues

### Critical Impact

### High Impact

- [x] ERROR_VAULT_NOT_CONTRACT shadows Vault state variable
- shadows:
• VaultRecoverable.ERROR_VAULT_NOT_CONTRACT
(@aragon/os/contracts/common/VaultRecoverable.sol#18)
- Recommendation: Rename the constant
- [ ] `_issueBounty()` seems a good candidate to refactor to mitigate risks and understanding the flow better. The potential issues include:
- [ ] Functions that send ether to arbitrary destinations (arbitrary-send):
- Finding: Projects._issueBounty (Projects.sol:803-856) sends eth to arbitrary user
• bounties.contribute.value(_bountySize)(address(this),bountyId,_bountySize) (Projects.sol#844-848)
- Recommendation: split branches into internal or reusable utility functions, ensure that an arbitrary user cannot withdraw unauthorize funds
- [ ] `_returnValueToVault()` suffers the same potential issue of sending eth to arbitrary user
- [ ] `getHash()` seems a good candidate to refactor, and seems out-of-scope of the Projects contract responsabilities, it could have potential issues:
- [ ] IPFS cIDs could eventually change its length so it is not safe to rely on it to perform byte operations, it could have unexpected results, also the cId string itself is not being guarded for checkings
- Recommendation: Use the existing multihash library to perform cIds checkings
- **Action Taken**: 46-char CID's likely won't be deprecated soon, so we'll keep using this current implementation. There's currently no cheap way to do checksums on-chain either. However, utilizing the [a multihash method](https://github.com/saurfang/ipfs-multihash-on-solidity) might be more robust and future-proof, but one of the limitations of solidity is the inability to pass objects in as arguments, and StandardBounties.sol creates significant stack depth limitations that prevent us from implementing a multihash struct if we want to maintain the ability to fund multiple bounties. However, `getHash` could be moved to a library contract.

- [x] `workSubmissions` has a getter but is never initialized and it seems there is no other place assigning values to it
- Recommendation: Refactor the use of `workSubmissions` to fix its lifecycle
- **Action Taken**: the submission lifecycle has been descoped from the projects contract and will occur through direct interaction with StandardBounties.sol

- [x] A sophisticated reentrancy attack could exploit `_removeBounty` to steal ethers
- Recommendation: Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy)
- **Action Taken**: Moved up `issue.hasBounty = false` to before the external contracts interaction, so the `require` will fail on reentrancy

### Medium impact

- [ ] bountyAllocator is the same Bounties state variable (the setting seems not currently changing the instance), might it be better trying to avoid the duplicity?

- [ ] Use mappings instead of arrays where possible to improve upgradeability as seen on other aragon apps

- [ ] Both `_issueBounty` and `_returnValueToVault` ignore return value from `ERC20Token.approve()`
- Recommendation: Refactor to use a require instead, this way we ensure it was effectively approved

- [ ] Potential issue at `initialize()` function: `_bountiesAddr` does not have any kind of checking, so it could be used by an attacker to spoof a malicious version of StandardBounties contract while still being compliant with the declared `Bounties` interface, fortunately, this is easily reversible by just changing settings, when detected on time
- Recommendation: Use checks to have more control over `_bountiesAddr` input

- [ ] A sophisticated reentrancy attack could exploit `reviewSubmission` without ethers theft (`issue.fulfilled`)
- Recommendation: Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy)

### Low impact

- [ ] Potential benign reentrancy in `Projects.addBounties` (Projects.sol:506-543):

        • standardBountyId =issueBounty(ipfsHash,_deadlines[i],_tokenContracts[i],_tokenTypes[i],_bountySizes[i]) (Projects.sol#526-532)
        • repos (Projects.sol#535-541)

- [ ] Potential bening reentrancy in `Projects.addBountiesNoAssignment` (Projects.sol:556-595):

        • standardBountyId = _issueBounty(ipfsHash,_deadlines[i],_tokenContracts[i],_tokenTypes[i],_bountySizes[i]) (Projects.sol#574-580)
        • repos (Projects.sol#583-589)
        • repos (Projects.sol#591)

- [ ] Some functions could potentially be declared as `external`:
- [ ] updateBounty
- [ ] removeBounties
- [ ] getSubmissionsLength
- [ ] getSubmission
- [ ] getApplicantsLength
- [ ] getApplicant
- [ ] curateIssues
- [ ] addBountiesNoAssignment
- [ ] addBounties
- [ ] Most functions could be declared as `isInitialized`

- [ ] Projects.ERROR_STANDARD_BOUNTIES_NOT_CONTRACT (Projects.sol:134) is never used in Projects

- [ ] SafeMath seems not used

### Comments and questions

- [ ] Is it strictly needed to keep 2 separate registry per id? Is it possible to encode the info into the IPFS object?
- [ ] Would it be better to be explicit about inheritances even if they are already inherited for readability? [this comment seems to encourage using the implicit inheritance](https://github.com/aragon/aragonOS/blob/c85d34e4bae0bf5b1ab78340b32e712d895179a7/contracts/apps/AragonApp.sol#L19)
- [ ] Should we consider using SafeERC20 as other aragon apps do instead this ERC20 interface?
- [ ] Should we consider extracting the assembly code into an utility library?
- [ ] Should we consider using EthTokenConstant as other aragon apps do?
- [ ] Would it worth to extract interfaces to external files?
- [ ] The current configuration makes it harder to isolate, follow and maintain configurations over the folders, as it is dependant on unpublished packages, increasing chance of bugs or side-effects
- Recommendation: Import every dependency contract when possible, and remove `@tps/test-helpers` dependency
- Recommendation: Use a simpler integration path for `StandardBounties`
- [ ] Consider following the standard comments pattern as recommended in the solidity guide
- [ ] Consider adding missing comments and some additional explanatory comments where needed
- [ ] Add missing license header
- [ ] Address comments at `function issueBounty`
- [ ] Does it make any difference to omit `address payable` in the `Bounties` interface (`contribute()` and `issueAndContribute` functions)? Since StandardBounties implementation has it
- [ ] Relayer implementation is pending
- [ ] Should we consider using already deployed contracts when possible?
- [ ] There could be some gas optimizations by pre-compute as we did on the other apps
- [ ] Tight variable packing: Consider sorting members of structs
Reference: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html
- [ ] Consider use constants instead of strings where possible
- [ ] Consider using `ethlint` instead of deprecated `solium` module
- [ ] Consider cleaning solidity-coverage config
- [ ] Remove unused imports added to other apps (BountiesEvents)
- [ ] Consider using 0xproject sol-compiler or crytic solc-select to handle different solidity versions
- [ ] Update to solidity 0.4.26 if possible (latest 0.4.x) and lock pragma
- [ ] Consider refactoring test file and making it possible to run them isolated from each other
- [ ] Clean unused migrations system
- [ ] 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
- [ ] Consider using `enum` where possible
- [ ] Generally the same as Rewards related to tests, migration files, ddependencies and imports, coverage and gas metrics configurations