###### tags: `Documentation and Standardization`

# D&S: Report style guide

Spearbit uses a markdown based template to collaboratively write the final report for security reviews. This markdown file is then converted to a PDF with minimal additional modifications.

- Use hackmd.io to collaborate on reports.
- Example of the markdown file: [Brink security review (engagement 2)](https://hackmd.io/@spearbit/HkddPdwtY).
- Example of a rendered [pdf](https://raw.githubusercontent.com/spearbit/portfolio/master/pdfs/Brink-Spearbit-Security-Review-Engagement-2.pdf).
- An example issue can be found in [Appendix](#Appendix-Example-finding-from-Brink-security-review).

# General suggestions
- The issue description should be detailed. Ideally, someone who is outside of the project and the security review team should be able to understand the issues, merely by reading the issue description.
- Sort by highest to lowest severity. For example, a critical bug that steals all the funds should appear before a generic suggestion such as floating pragma.
- Don't number issues; the LaTeX template will generate section numbers.
- For issues with the same severity, the most unique and specific one should appear first. For example, a specific gas optimization should come before a generic one such as change `memory` to `calldata`.
- Group issues together, whenever that's relevant. For example, if a certain gas optimization is relevant throughout the codebase, keep it as a single issue.
- Avoid screenshots. Prefer text whenever possible, for example, for code snippets, specification, etc.
- Consider minimizing code examples for the most relevant parts.
- For links to a hosted git source code, use permalinks (to specific commit hashes).
- Avoid raw links if possible, i.e, prefer [`NoDelegateCall.sol#L18`](https://github.com/Uniswap/v3-core/blob/62d65bf88c4fb23671104c28f5bcae566274cb15/contracts/NoDelegateCall.sol#L18) instead of https://github.com/Uniswap/v3-core/blob/62d65bf88c4fb23671104c28f5bcae566274cb15/contracts/NoDelegateCall.sol#L18.
- Consider using diffs while making code recommendations:

modified   brink-core/contracts/Account/Account.sol
@@ -160,7 +160,7 @@ contract Account is EIP712SignerRecovery, EIP1271Validator {
   function proxyOwner() internal view returns (address _proxyOwner) {
     assembly {
       extcodecopy(address(), mload(0x40), 0x2d, 0x14)
-      _proxyOwner := mload(sub(mload(0x40), 0x0c))
+      _proxyOwner := shr(0x60, mload(mload(0x40)))
     }
   }

- Highlight any references to code (function names, variable names, etc.) as code text (i.e. denoted with backticks).

# LaTeX

- A typical latex issue is `\textt` overflowing the margins (equivalent of `backticks` in markdown). For example `aVeryLongVariableName` might overflow the margin. A trivial fix is to manually add hyphen after looking at the PDF output, say `aVeryLongVariable-Name`, which would make LaTeX prevent this overflow.

# Appendix (Example finding from Brink security review)

### Risk of replay attacks across chains

**Severity**: Medium Risk, Gas Optimization

**Context**: [`EIP712SignerRecovery.sol#L12-L14`](https://github.com/brinktrade/brink-core/blob/c5b831d9e0239ff119034403bb93cae17ddd4590/contracts/Account/EIP712SignerRecovery.sol#L12-L14)

Currently, for deploying on EVM compatible chains, the unique identifier `_chainId` is specified by Brink. The users need to trust the deployer, i.e. Brink to have unique values of `_chainId` for different chains. If the deployer violates this condition, there is a risk of replay attacks, where signed messages on one chain may be replayed on another chain.

constructor(uint256 _chainId) EIP7125SignerRecovery(chainId_){ ... }

**Recommendation**: Read `_chainId` on chain directly, using `block.chainid` (available from Solidity 0.8.0) or using `chainid()` in inline assembly for older solidity versions, rather than as a constructor parameter. It is also a gas optimization (saves 1 gas).

**Brink**: Fixed in commit [feef7d9](https://github.com/brinktrade/brink-core/commit/feef7d92d2888e1ee062038f55a7301a2300ba3e). There is potential risk that a chain could hardfork a change to the `chainID` value, which would invalidate all signed messages. The main benefit and reason for the fix was so the `Account.sol` address can be consistent across all chains. Since `Proxy.sol` now sets this address to the `ACCOUNT_IMPLEMENTATION` constant, this keeps all Proxy addresses consistent across all chains as well.

**Spearbit**: Resolved. We think that a hardfork / protocol-upgrade that changes the value of `chainid` is extremely unlikely on almost all big chains, therefore this risk is acceptable.

<br>