**dpd-assembly review**
by @devtooligan
# **Repository.huff**
**(INFORMATIONAL)** **l2** Address.huff imported but not used
**(INFORMATIONAL)** **l5-18** Interface
- missing `owners(uint256)`
- incorrect fn sig sb `addDpd(uint256,bytes32,address,address)`
- extranneous fn `updateDpd(uint256,bytes32)`
- missing `updateDpdData(uint256, bytes32)`
- incorrect fn name `setDpdOwner` sb `updateDpdOwner`
- incorrect fn name `setDpdUpdater` sb `updateDpdUpdater`
- inccorect event sig sb `DPDUpdated(uint,bytes32)`
- incorrect event name `DPDOwnerChanged` sb `DPDOwnerUpdated`
- incorrect event name `DPDUpdaterChanged` sb `DPDUpdaterUpdated`
**(INFORMATIONAL)** **l25** Incorrect event sig in comment sb `DPDUpdated(uint,bytes32)`
**(INFORMATIONAL)** **l28-29** Should use correct fn name in comment and as constant name for consistency. sb `DPDOwnerUpdated` and `DPD_OWNER_UPDATED` respectively
**(INFORMATIONAL)** **l31-32** Should use correct fn name in comment and as constant name for consistency. sb `DPDUpdaterUpdated` and `DPD_UPDATER_UPDATED` respectively
**(INFORMATIONAL)** **l35** Unclear/inconsistent inline comment - Is there a reason only this one has a comment after?
**(INFORMATIONAL)** **l36** Unused storage pointer `DPD_ARRAY_LENGTH_SLOT` - related to l72
**(INFORMATIONAL)** **l44 & l49** Inconsistent comments - The two similar, subsequent view functions don't have the same comments as this fn.
**(INFORMATIONAL)** **l51** Inconsistent with subsequent 2 view fns which put the return on the next line.
**(MEDIUM)** **l71** Missing view functions:
- Consider adding a `GET_ARRAY` function that returns the entire array. [see Solcurity Standard V9](https://github.com/Rari-Capital/solcurity)
- Consider adding a `TOTAL_SUPPLY` function that returns the total number of DPD's (the length of the array).
**(CRITICAL)** **l74** `ADD_DPD`
Function comment says `Get the DPD array length, which serves as the DPD ID.` but in actuality, an index is passed in. **NOTE: SEE BELOW RECOMMENDATION FOR OVERHAULING THIS FUNCTION AND REPLACING WITH ADD_DPD() AND UPDATE_DPD()**
**(INFORMATIONAL)** **l95-104** For some reason in the stack comment `length*32, length` gets added to the end.
**(INFORMATIONAL)** **l117** Confirming we don't allow zero's for any of the fields `updater`, `data`, or `owner` (so no "burning" by sending to zero address).
**(INFORMATIONAL)** **l144** `UPDATE_DPD` - Consider renaming this to `UPDATE_DPD_DATA` or `UPDATE_DPD_CID` to be consistent.
**(HIGH)** **l144** We revert if any zeros are found during ADD_DPD but we allow zeros to go through here and in the other single attribute update fns. Recommend iszero check for all 3 update functions.
**(INFORMATIONAL)** **l158-l170** Consider abstracting this logic to a macro `ONLY_OWNER_OR_UPDATER`.
**(MEDIUM)** **l199** Recommend allowing owner to change `updater`.
**(INFORMATIONAL)** **l233** Consider allowing the `updater` to update the `owner` as well. This would allow for an approved `udpater` proxy to transfer the DPD on behalf of the owner.
**(GAS OPTIMIZATION)** **l260** The fns in MAIN() could be sorted, transactional first then sorted by most often called. View functions that would likely be viewed offchain can go last.
**RECOMMENDATION FOR OVERHAULING ADD_DPD()**
```
/* DPD Overwrite existing */
/// @dev 🔫 This low level fn has no auth, validation, or other error checking
#define macro SET_DPD() = takes(0) returns (0) {
// Store the DPD data.
0x24 calldataload // [data, dpd_id]
dup2 // [dpd_id, data, dpd_id]
[DPD_ARRAY_SLOT] // [salt, dpd_id, data, dpd_id]
STORE_ELEMENT_FROM_KEYS(0x00) // [dpd_id]
// Store the address of the owner.
0x44 calldataload // [owner, dpd_id]
dup2 // [dpd_id, owner, dpd_io]
[DPD_OWNER_MAP_SLOT] // [salt, dpd_id, owner, dpd_io]
STORE_ELEMENT_FROM_KEYS(0x00) // [dpd_id]
// Store the address of the updater.
0x64 calldataload // [updater, dpd_id]
dup2 // [dpd_id, updater, dpd_id]
[DPD_UPDATER_MAP_SLOT] // [salt, dpd_id, updater, dpd_id]
STORE_ELEMENT_FROM_KEYS(0x00) // [dpd_id]
/* DPD Add New */
#define macro ADD_DPD() = takes(0) returns (0) {
// Get the DPD array length, which serves as the DPD ID.
[DPD_ARRAY_LENGTH_SLOT] // [salt]
LOAD_ELEMENT(0x00) // [dpd_id]
dup1 // [dpd_id, dpd_id]
// Update array length
0x1 add // [dpd_id + 1, dpd_id]
[DPD_ARRAY_LENGTH_SLOT] // [salt, dpd_id + 1, dpd_id]
STORE_ELEMENT(0x00) // [dpd_id]
// Checks
ID_CHECKS(error) // [dpd_id]
// Emit the event.
// DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)
dup1 // [dpd_id, dpd_id]
[DPD_ADDED_EVENT_SIGNATURE] // [sig, dpd_id, dpd_id]
0x44 calldataload 0x00 mstore // [sig, dpd_id, dpd_id]
0x64 calldataload 0x20 mstore // [sig, dpd_id, dpd_id]
0x24 calldataload 0x40 mstore // [sig, dpd_id, dpd_id]
0x60 0x00 // [offset, size, sig, dpd_id, dpd_id]
log2 // [dpd_id]
SET_DPD() // [dpd_id]
// Stop the program.
stop
// Revert if the checks are wrong.
error:
0x00 0x00 revert
}
/* DPD Edit */
#define macro UPDATE_DPD() = takes(0) returns (0) {
0x04 calldataload // [dpd_id]
// Checks
// by checking isOwner we also get an in bounds check
ONLY_OWNER_OR_UPDATER() // [dpi_id]
ID_CHECKS(error) // [dpd_id]
// Emit the event.
// DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)
dup1 // [dpd_id, dpd_id]
[DPD_UPDATED_EVENT_SIGNATURE] // [sig, dpd_id, dpd_id]
0x44 calldataload 0x00 mstore // [sig, dpd_id, dpd_id]
0x64 calldataload 0x20 mstore // [sig, dpd_id, dpd_id]
0x24 calldataload 0x40 mstore // [sig, dpd_id, dpd_id]
0x60 0x00 // [offset, size, sig, dpd_id, dpd_id]
log2 // [dpd_id]
SET_DPD() // [dpd_id]
// Stop the program.
stop
// Revert if the checks are wrong.
error:
0x00 0x00 revert
}
```
# **Repository.t.sol -- TESTS**
**Interface**
- **(INFORMATIONAL)** Recommend to split up the interface into events and functions. Not only does it make it easier to import events into the test ctrct, but [there's an argument for not including events in interfaces](https://twitter.com/alcueca/status/1466152649015836673?s=20&t=iaEVlpqN6NEWCsVi_c2NSg). Rename interface `Repository` to `IRepository` and move all events from the newly renamed `IRepository` to a new abstract contract `RepositoryEvents`. Can move these two to a new file or to the top of test file.
- **(INFORMATIONAL)** Have the test contract inherit `RepositoryEvents` and delete the duped events currently on **l9-l12** within the test contract.
- **(MEDIUM)** All `address` args in all events should be indexed. This will mean updating the `log` code in the contract and the `vm.expectEmit` code in the tests as well. [see Solcurity Standard E5](https://github.com/Rari-Capital/solcurity)
- **(INFORMATIONAL)** Incorrect natspec for event`DPDUpdated` -- sb `Event emitted when a DPD's CID is updated.`
- **(INFORMATIONAL)** Typo in natspec for event`DPDUpdated` -- "upgrader"
- **(INFORMATIONAL)** Consider adding the param names to the signatures for readability like in contract in comment on l79 `DPDAdded(uint256 indexed dpdId, address owner, address updater, bytes32 cid)`
**Tests**
- **(HIGH)** Auth checks are not tested.
- **(MEDIUM)** View fns not tested.
- **(HIGH)** Unit testing (or fuzzing) for ID_CHECKS.
- **(MEDIUM)** Currently only one event `DPDAdded` is tested. There should be at least 1 test for each event.
- **(MEDIUM)** Ensure that there is a test case for every where that the contract could revert.
- **(INFORMATIONAL)** The setup for the `vm.expectRevert` is incorrect in its current state. Since there is only 1 topic area so it should be `vm.expectRevert(true, false, false, true)`
- **(INFORMATIONAL)** The assertion made in the comment on **l45** is not entirely true. `@dev Ensure that you cannot create two DPDs with the same ID.` You can create a DPD with id of 1 and update all fields to zeros for `data`, `owner`, and `updater`. You could then call `ADD_DPD` again with the same dpd_id and it would not revert. Prefer `vm.expectRevert` to `testFail` for tighter control and less false positives.
Sign in with Wallet
Connect another wallet