Skip to content

Commit f043144

Browse files
aiagentdev-1990aiagentdev-1990
authored
docs: Add private audit (#79)
1 parent b896266 commit f043144

File tree

1 file changed

+293
-0
lines changed

1 file changed

+293
-0
lines changed

‎audits/hoku-private-audit.md

+293
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,293 @@
1+
# Hoku Audit Prep
2+
3+
# Overview
4+
5+
| Project Name | Hoku |
6+
| --- | --- |
7+
| Repositories | [https://github.com/hokunet/contracts/tree/main](https://github.com/hokunet/contracts/tree/main) [https://github.com/hokunet/ipc/tree/main/contracts](https://github.com/hokunet/ipc/tree/main/contracts) |
8+
| Commits | 5a6710409a90944ceb3ff4d8ad9edea1b00557c3, 786b6fc6678ff30addcd033b85f55dfd107e4e1c|
9+
| Language | Solidity |
10+
| Scope | All Solidity contracts in https://github.com/hokunet/contracts/tree/main <br>**Excluding**: `BucketManger.sol`, `Credit.sol`, `CreditManager.sol`, `BlobManager.sol` <br> All Solidity contracts in https://github.com/hokunet/ipc/tree/main/contracts |
11+
12+
## Process
13+
14+
- **Static Analysis:** Auditor ran [Slither](https://github.com/crytic/slither) on the codebase to identify common vulnerabilities
15+
- **Manual Code Review:** Auditor ****manually reviewed the code to identify areas not following best practices and to catch potential vulnerabilities
16+
17+
# Audit Prep Suggestions
18+
19+
This section provides several suggestions on improving the code documentation in the document [here](https://www.notion.so/15adfc9427de8012a8dafd7faae5eadc?pvs=21).
20+
21+
| **Suggestion** | **Reasoning** |
22+
| --- | --- |
23+
| Add interaction diagrams outlining how the off-chain and on-chain components interact with each other. These diagrams should list out what function calls each component makes to each other and what they return. <br> I suggest using [Mermaid](https://www.mermaidchart.com/) to do this | It was not immediately clear how the functions on the contracts would be called. It would make it easier for auditors to onboard if they were given a clearer description on how the different processes worked. For instance it is not immediately clear when `interceptPowerDelta` is called [here](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L74). |
24+
| Add a Glossary at the beginning of the document | A glossary will help auditors onboard as they will have a clear definition of each of the key terms. |
25+
26+
# Findings
27+
28+
## Low
29+
30+
### L1: Missing handling of `token.transfer` return value
31+
32+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L164](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L164)
33+
34+
The `notifyValidClaim` function in `ValidatorRewarder` makes a `transfer` call. As per the ERC20 specs, some ERC20 tokens will not revert if the `transfer` call fails but will instead return `false`. This can lead to recipients not correctly receiving any Hoku tokens when `notifyValidClaim` is called. This scenario seems unlikely to happen with the current Hoku implementation as it uses OpenZeppelin’s ERC20 token implementation, which will either return `true` if the transfer succeeds or reverts if the transfer fails. The team should however be careful if they ever upgrade the Hoku token contract’s implementation.
35+
36+
**Recommendation:** Use OpenZeppelin’s [SafeERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol) library and perform transfers using `safeTransfer`
37+
38+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR173).
39+
40+
### L2: `setInflationRate` missing validations
41+
42+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L106](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L106)
43+
44+
The `setInflationRate` function does not validate the new inflation rate. This can cause problems if the contract owner accidentally sets the inflation rate to 0 or an extremely large number.
45+
46+
**Recommendation:** Allow the owner to set a minimum and maximum inflation rate in the contract and ensure that the new inflation rate `rate` is within the boundaries.
47+
48+
**Resolution:** The team has acknowledged this and have opted to remove the `setInflationRate` function
49+
50+
### L3: `initialize` function does not validate Hoku token address
51+
52+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L65](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L65)
53+
54+
The `initialize` function does not validate the `hokuToken` address that is being set.
55+
56+
**Recommendation:** Validate that the `token` address is not the zero address
57+
58+
**Resolution**: The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR71)
59+
60+
### L4: **`notifyValidClaim` function allows tokens to be minted to the zero address**
61+
62+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L115](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L115)
63+
64+
The `notifyValidClaim` function does not check that the `data.validator` address is the zero address. This could lead to tokens being accidentally burnt by sending them to the zero address.
65+
66+
**Recommendation:** Revert if data.validator is the zero address or check that data.validator is a valid validator address by querying the isAllow function on the ValidatorGater contract.
67+
68+
**Resolution:** The team ha addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR126)
69+
70+
### L5: Missing Subnet validation
71+
72+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L55](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L55)
73+
74+
`setSubnet` does not validate that the addresses in the `route` array are not the zero address and that the length of the `route` array is greater than 0.
75+
76+
**Recommendation:** Ensure that the `route` array’s length is greater than 0 and that the addresses in `route` are not the zero address or leave a comment explaining what the zero address represents.
77+
78+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-b4781b8ea78276fa0f2a3cd3eb429c296c7e6569f0deb7fdd4e9356d8e77b353R61-R69)
79+
80+
### L6: Drip Amount drips an incorrect amount
81+
82+
[https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L30](https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L30)
83+
84+
The `_dripAmount` should be `18 ether` assuming it’s 18 decimal places or `18 * 10 ** tokenDecimals` . This issue has been marked as low as the contract owner can call `setDripAmount` to correct it after the contract has been deployed.
85+
86+
**Recommendation:** Assign `_dripAmount` in terms of whole token units e.g `18 ether` if the Hoku token is 18dp.
87+
88+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-2dc9f797f451bc2a3ac43046dd7d72cf66ad5a9c87bf0c4e86d81b969a2be1acR34). They have also updated the `_dripAmount` to `5 ether` from `18` wei.
89+
90+
## Informational
91+
92+
### I1: Reentrancy in `ValidatorRewarder` `notifyValidClaim`
93+
94+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L157](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L157)
95+
96+
The `notifyValidClaim` function in `ValidatorRewarder` calls [`token.mint`](http://token.mint) twice before it updates sets `latestClaimedCheckpoint`. A malicious `token` address could exploit this by reentering the `notifyValidClaim` call to potentially mint an infinite amount of tokens to itself without any additional guardrails. This scenario however is prevented because of two reasons
97+
98+
1. The `token` address is set to the address of the Hoku token by a trusted actor at initialization by a trusted actor and cannot be changed after
99+
2. `checkpointToSupply` is set to the total supply of the Hoku tokens prior to calling `mint` meaning that the reentering call will not try to mint more tokens.
100+
101+
It is nevertheless best practice to always set storage variables prior to making any external calls.
102+
103+
**Recommendation:** Set `latestClaimedCheckpoint` before calling `token.mint` or add a comment explaining why there is no reentrancy vulnerability
104+
105+
**Resolution:** The team has addressed this by setting `latestClaimedCheckpoint` before calling `token.mint` [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR157)
106+
107+
### I2: Missing Mapping Names
108+
109+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L46](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L46)
110+
111+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L26](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L26)
112+
113+
Map keys and values should be named to improve readability so that readers know what the keys and values represent.
114+
115+
```solidity
116+
mapping(uint64 => uint256) public checkpointToSupply;
117+
118+
can be named
119+
120+
mapping(uint64 checkpointId => uint256 totalSupply) public checkpointToSupply;
121+
```
122+
123+
**Recommendation:** Add names to `mapping` keys and values
124+
125+
**Resolution:** The team has addded names to the `mapping` keys and values
126+
[here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR44)
127+
128+
### I3: Missing event emissions in state changes
129+
130+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L72](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L72)
131+
132+
https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L55
133+
134+
It is generally best practice to emit events whenever there is a state change so that they can be indexed off-chain.
135+
136+
**Recommendation:** Emit events whenever storage variables are set.
137+
138+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR90)
139+
140+
### I4: Inconsistent function returns
141+
142+
Some functions return unnamed variables
143+
144+
[https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L126](https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L126)
145+
146+
Other functions return named variables
147+
148+
[https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L104](https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L104)
149+
150+
It is best practice to be consistent throughout the codebase
151+
152+
**Recommendation:** Be consistent with naming function return variables
153+
154+
**Resolution:** The team has acknowledged this and have opted to keep
155+
the current return naming
156+
157+
### I5: Unnecessary `<` check
158+
159+
[https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L51](https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L51)
160+
161+
It is unnecessary to check that `msg.value <= 0` as it can just be `msg.value == 0`. `msg.value` is a `uint256` that can never be below 0.
162+
163+
**Recommendation:** Update `msg.value <= 0` to `msg.value == 0`
164+
165+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-2dc9f797f451bc2a3ac43046dd7d72cf66ad5a9c87bf0c4e86d81b969a2be1acR55)
166+
167+
### I6: Inefficient `drip` function
168+
169+
[https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L62C5-L76C8](https://github.com/hokunet/contracts/blob/main/src/token/Faucet.sol#L62C5-L76C8)
170+
171+
The `drip` function can be optimized by
172+
173+
1. Only using a single for loop and declare loop more efficiently
174+
2. Performing the `address(this).balance < amount` check first to terminate the function earlier.
175+
3. Handling the response from `recipient.transfer`
176+
177+
```solidity
178+
function drip(
179+
address payable recipient,
180+
string[] calldata keys
181+
) external onlyOwner {
182+
183+
/// Terminate function early
184+
if (address(this).balance < amount) {
185+
revert FaucetEmpty();
186+
}
187+
188+
uint256 keysLength = keys.length;
189+
uint256 amount = _dripAmount;
190+
191+
/// Optimize for loop to save gas
192+
/// 1. No need to initialize i = 0 as i is already initialized
193+
/// to 0
194+
/// 2. Preincrement i instead of postincrementing by doing ++i instead
195+
/// of i++. Preincrementing is cheaper than postincrementing
196+
for (uint256 i; i < keysLength; ++i) {
197+
if (_nextRequestAt[keys[i]] > block.timestamp) {
198+
revert TryLater();
199+
}
200+
_nextRequestAt[keys[i]] = block.timestamp + (12 hours);
201+
}
202+
203+
/// Revert if transfer not successful
204+
(bool success,) = recipient.transfer(amount);
205+
206+
if (!success) revert TransferNotSuccessful(recipient);
207+
}
208+
```
209+
210+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-2dc9f797f451bc2a3ac43046dd7d72cf66ad5a9c87bf0c4e86d81b969a2be1acR72-R85)
211+
212+
### I7: Declare roles as constants
213+
214+
[https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L33](https://github.com/hokunet/contracts/blob/main/src/token/Hoku.sol#L33)
215+
216+
Declaring roles as constants can save gas. Constant variables are baked into the contract’s bytecode at compile time meaning that the contract won’t have to access storage to access it.
217+
218+
```solidity
219+
bytes32 public ADMIN_ROLE; // solhint-disable-line var-name-mixedcase
220+
bytes32 public MINTER_ROLE; // solhint-disable-line var-name-mixedcase
221+
222+
can be declared as
223+
224+
bytes32 public constant ADMIN_ROLE = keccak256('ADMIN_ROLE');
225+
bytes32 public constant MINTER_ROLE = keccak256('MINTER_ROLE');
226+
227+
instead of declaring these in the initialize function
228+
```
229+
230+
**Recommendation:** Store values constant values as `constant`.
231+
232+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-7442c860c975851e8fe0043fad4d1208282cbd29ddf95ef9b0724ac720db99cbR33-R34)
233+
234+
### I8: Unnecessary `uint64` storage variable
235+
236+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L33](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L33)
237+
238+
`latestClaimedCheckpoint` can be stored as `uint256` instead of `uint64`. Operations using `latestClaimedCheckpoint` will actually cost more gas if it is stored as `uint64` as Solidity will need to convert it into `uint256` internally before using it in any calculations as per Solidity’s [documentation](https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#layout-of-state-variables-in-storage).
239+
240+
**Recommendation:** Either store `latestClaimedCheckpoint` as `uint256` or pack `latestClaimedCheckpoint`, `inflationRate` and `checkpointPeriod` into a single struct such as the one below.
241+
242+
```solidity
243+
struct ValidatorRewarderStorage {
244+
uint64 latestClaimedCheckpoint;
245+
uint64 checkpointPeriod;
246+
uint128 inflationRate;
247+
}
248+
249+
Assuming that the values will fit into the declared uints.
250+
```
251+
252+
**Resolution:** The team has acknowledged this and have opted to keep it
253+
as `uint64` to match Filecoin's epoch height type. They have left a comment
254+
explaining their decision [here](https://github.com/recallnet/contracts/pull/57/files#diff-4889ed1a3017fce5f4c08e5d132444ed9bee3fc43a238744441c754ce76ebcbfR36)
255+
256+
### I9: Pack `PowerRange` struct
257+
258+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L13](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorGater.sol#L13)
259+
260+
Currently the `PowerRange` struct is declared as
261+
262+
```solidity
263+
struct PowerRange {
264+
uint256 min;
265+
uint256 max;
266+
}
267+
```
268+
269+
This declaration causes the struct to use 2 slots. Depending on what the largest value of `max` will be, the struct can be stored more efficiently by declaring `min` and `max` as `uint128` so that it only takes up one storage space.
270+
271+
```solidity
272+
struct PowerRange {
273+
uint128 min;
274+
uint128 max;
275+
}
276+
```
277+
278+
**Recommendation:** Pack the `PowerRange` struct so that it only takes up a single storage space.
279+
280+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-b4781b8ea78276fa0f2a3cd3eb429c296c7e6569f0deb7fdd4e9356d8e77b353R14-R16)
281+
282+
### I10: Reverting in `whenActive` modifier
283+
284+
[https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L83](https://github.com/hokunet/contracts/blob/main/src/token/ValidatorRewarder.sol#L83)
285+
286+
Currently the `whenActive` modifier will skip the function execution instead of reverting when the contract is in an inactive state. Consider reverting instead of skipping execution to provide a better user experience as:
287+
288+
1. Off-chain simulators will revert and prevent the caller from accidentally executing a transaction that is not expected to do anything
289+
2. Unused gas is refunded to the caller.
290+
291+
**Recommendation:** Revert when the contract is inactive
292+
293+
**Resolution:** The team has addressed this [here](https://github.com/recallnet/contracts/pull/57/files#diff-b4781b8ea78276fa0f2a3cd3eb429c296c7e6569f0deb7fdd4e9356d8e77b353R55)

0 commit comments

Comments
 (0)