_Follow along with the video lesson:_ --- ### Recon Before we even look at the code, we can get a better sense of things by running our static analysis tools: Aderyn and Slither. The Boss Bridge repo includes make commands for us! ```bash make slither ```  Oh .. ok, wow, we've got some work to do. We'll .. go through these later đ Let's try: ```bash make aderyn ``` Aderyn provides us with a beautiful formatted report.md with all of it's findings! <details> <summary>Aderyn report.md</summary> # Aderyn Analysis Report This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a static analysis tool built by [Cyfrin](https://cyfrin.io), a blockchain security company. This report is not a substitute for manual audit or security review. It should not be relied upon for any purpose other than to assist in the identification of potential security vulnerabilities. # Summary ## Files Summary | Key | Value | | ----------- | ----- | | .sol Files | 4 | | Total nSLOC | 101 | ## Files Details | Filepath | nSLOC | | -------------------- | ------- | | src/L1BossBridge.sol | 64 | | src/L1Token.sol | 8 | | src/L1Vault.sol | 12 | | src/TokenFactory.sol | 17 | | **Total** | **101** | ## Issue Summary | Category | No. of Issues | | -------- | ------------- | | High | 1 | | Low | 6 | # High Issues ## H-1: Arbitrary `from` passed to `transferFrom` (or `safeTransferFrom`) Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) can lead to loss of funds, because anyone can transfer tokens from the `from` address if an approval is made. - Found in src/L1BossBridge.sol [Line: 74](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L74) ```solidity token.safeTransferFrom(from, address(vault), amount); ``` # Low Issues ## L-1: Centralization Risk for trusted owners Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds. - Found in src/L1BossBridge.sol [Line: 27](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L27) ```solidity contract L1BossBridge is Ownable, Pausable, ReentrancyGuard { ``` - Found in src/L1BossBridge.sol [Line: 49](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L49) ```solidity function pause() external onlyOwner { ``` - Found in src/L1BossBridge.sol [Line: 53](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L53) ```solidity function unpause() external onlyOwner { ``` - Found in src/L1BossBridge.sol [Line: 57](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L57) ```solidity function setSigner(address account, bool enabled) external onlyOwner { ``` - Found in src/L1Vault.sol [Line: 12](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Vault.sol#L12) ```solidity contract L1Vault is Ownable { ``` - Found in src/L1Vault.sol [Line: 19](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Vault.sol#L19) ```solidity function approveTo(address target, uint256 amount) external onlyOwner { ``` - Found in src/TokenFactory.sol [Line: 11](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/TokenFactory.sol#L11) ```solidity contract TokenFactory is Ownable { ``` - Found in src/TokenFactory.sol [Line: 23](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/TokenFactory.sol#L23) ```solidity function deployToken(string memory symbol, bytes memory contractBytecode) public onlyOwner returns (address addr) { ``` ## L-2: Unsafe ERC20 Operations should not be used ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. - Found in src/L1BossBridge.sol [Line: 99](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L99) ```solidity abi.encodeCall(IERC20.transferFrom, (address(vault), to, amount)) ``` - Found in src/L1Vault.sol [Line: 20](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Vault.sol#L20) ```solidity token.approve(target, amount); ``` ## L-3: Missing checks for `address(0)` when assigning values to address state variables Check for `address(0)` when assigning values to address state variables. - Found in src/L1BossBridge.sol [Line: 58](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L58) ```solidity signers[account] = enabled; ``` - Found in src/L1Vault.sol [Line: 16](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Vault.sol#L16) ```solidity token = _token; ``` ## L-4: `public` functions not used internally could be marked `external` Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. - Found in src/TokenFactory.sol [Line: 23](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/TokenFactory.sol#L23) ```solidity function deployToken(string memory symbol, bytes memory contractBytecode) public onlyOwner returns (address addr) { ``` - Found in src/TokenFactory.sol [Line: 31](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/TokenFactory.sol#L31) ```solidity function getTokenAddressFromSymbol(string memory symbol) public view returns (address addr) { ``` ## L-5: Event is missing `indexed` fields Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. - Found in src/L1BossBridge.sol [Line: 40](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L40) ```solidity event Deposit(address from, address to, uint256 amount); ``` - Found in src/TokenFactory.sol [Line: 14](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/TokenFactory.sol#L14) ```solidity event TokenDeployed(string symbol, address addr); ``` ## L-6: PUSH0 is not supported by all chains Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. - Found in src/L1BossBridge.sol [Line: 15](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1BossBridge.sol#L15) ```solidity pragma solidity 0.8.20; ``` - Found in src/L1Token.sol [Line: 2](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Token.sol#L2) ```solidity pragma solidity 0.8.20; ``` - Found in src/L1Vault.sol [Line: 2](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Vault.sol#L2) ```solidity pragma solidity 0.8.20; ``` - Found in src/TokenFactory.sol [Line: 2](https://github.com/Cyfrin/7-boss-bridge-audit/blob/d59479cbf925f281ef79ccb35351c4ddb002c3ef/src/L1Token.sol#L2) ```solidity pragma solidity 0.8.20; ``` </details> Read through the Aderyn report and you'll likely see a number of identified vulnerabilities we've seen before, great feedback from Aderyn. We'll definitely be adding some of those findings to our report. ### Wrap Up We've read the documentation, gained some context, run our static analysis tools and it's time to start some deeper recon. In the past we've applied "The Tincho" as a pragmatic approach to our audits, but this time let's attempt to apply "The Hans Checklist" and systematically check this protocol with a different approach entirely. Let's go over the Checklist, in the next lesson!
Follow along with the video lesson:
Before we even look at the code, we can get a better sense of things by running our static analysis tools: Aderyn and Slither.
The Boss Bridge repo includes make commands for us!
Oh .. ok, wow, we've got some work to do. We'll .. go through these later đ
Let's try:
Aderyn provides us with a beautiful formatted report.md with all of it's findings!
This report was generated by Aderyn, a static analysis tool built by Cyfrin, a blockchain security company. This report is not a substitute for manual audit or security review. It should not be relied upon for any purpose other than to assist in the identification of potential security vulnerabilities.
Key | Value |
---|---|
.sol Files | 4 |
Total nSLOC | 101 |
Filepath | nSLOC |
---|---|
src/L1BossBridge.sol | 64 |
src/L1Token.sol | 8 |
src/L1Vault.sol | 12 |
src/TokenFactory.sol | 17 |
Total | 101 |
Category | No. of Issues |
---|---|
High | 1 |
Low | 6 |
from
passed to transferFrom
(or safeTransferFrom
)Passing an arbitrary from
address to transferFrom
(or safeTransferFrom
) can lead to loss of funds, because anyone can transfer tokens from the from
address if an approval is made.
Found in src/L1BossBridge.sol Line: 74
Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.
Found in src/L1BossBridge.sol Line: 27
Found in src/L1BossBridge.sol Line: 49
Found in src/L1BossBridge.sol Line: 53
Found in src/L1BossBridge.sol Line: 57
Found in src/L1Vault.sol Line: 12
Found in src/L1Vault.sol Line: 19
Found in src/TokenFactory.sol Line: 11
Found in src/TokenFactory.sol Line: 23
ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library.
Found in src/L1BossBridge.sol Line: 99
Found in src/L1Vault.sol Line: 20
address(0)
when assigning values to address state variablesCheck for address(0)
when assigning values to address state variables.
Found in src/L1BossBridge.sol Line: 58
Found in src/L1Vault.sol Line: 16
public
functions not used internally could be marked external
Instead of marking a function as public
, consider marking it as external
if it is not used internally.
Found in src/TokenFactory.sol Line: 23
Found in src/TokenFactory.sol Line: 31
indexed
fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
Found in src/L1BossBridge.sol Line: 40
Found in src/TokenFactory.sol Line: 14
Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.
Read through the Aderyn report and you'll likely see a number of identified vulnerabilities we've seen before, great feedback from Aderyn. We'll definitely be adding some of those findings to our report.
We've read the documentation, gained some context, run our static analysis tools and it's time to start some deeper recon.
In the past we've applied "The Tincho" as a pragmatic approach to our audits, but this time let's attempt to apply "The Hans Checklist" and systematically check this protocol with a different approach entirely.
Let's go over the Checklist, in the next lesson!
Code Review - Static Analysis Tools & Fixing Issues. Patrick leverages Slither and Aderyn to dive into Boss Bridge.
Previous lesson
Previous
Next lesson
Next
Give us feedback
Duration: 25min
Duration: 1h 18min
Duration: 35min
Duration: 2h 28min
Duration: 5h 03min
Duration: 5h 22min
Duration: 4h 33min
Duration: 2h 01min
Duration: 1h 40min