-
Notifications
You must be signed in to change notification settings - Fork 2
/
Yshurik-Audit.txt
118 lines (83 loc) · 7.31 KB
/
Yshurik-Audit.txt
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
Audit:
The purpose of this audit was to indicate the correctness of the contract's operation, check the quality of the code and general help in further development.
This audit covered the solidity code at the commit ba8a4ab10149b6bbf3f11566e7172f44d6d4b7a1. No logic or security flaws have been found.
Disclosure:
This audit was conducted on 2024-03-09 on code retrieved from the client on the days prior.
Summary:
Overall the auditor found the smart contract well written and no vulnerabilities were found. No logic or security flaws have been found. There are given recommendations to improve efectiveness of routines, logic flows and operations in the smart contracts, add migration interfaces for possible further smart contract upgrades. It is opinion of the auditor that the contract is safe from exploits.
Low Severity:
timelimits validity is not checked
The validity of timelimits is completely dependent on frontend implementation. If zeros or too small then dde-contract is immediately outdated. As nature of solidity contracts allows to connect any 3rd-party frontend this case could be considered and minimum validation of timelimits to be done in the contract codes.
requestExtension call depends completely on block.timestamp
There is one business-logic-important method requestExtension has the critical dependency to the block.timestamp . Such logic can be a subject of the case when ethereum mempool is over-crowed and transactions are stuck (up to 2 days and if not mined they removed from the mempool). There is possibility to lose the agreed extension even if consensus is reached by all parties of the business.
It can be considered the second/alternative way of executing requestExtension by getting data from last_resort_proof_contract which can be an oracle or bridge to get the proof of the dde-contract extension from the other chain. Then the extension can be reached even after passing the deadline due to technical issues.
Example: ETH chain is over-crowded, so requestExtension data is published on BAY chain, after the mempool relaxed (but deadline is passed), the ETH requestExtension is called and picked up the data from bridge contract to make the extension happen.
DEV NOTE:
Time limits should be recommended to be at least a few weeks in the front end however it was unclear if this was appropriate to enforce this in
Solidity. This raises interesting questions on the best way for users to interact. It's typically not recommended to be able to rescue expired
contracts due to collusion or extortion concerns however extra escrow options may be considered in the future. Thanks for the recommendations!
Specific concerns
Default affiliate fee collector
By default, when there is no affiliate set explicit, it is seen the fees are just burn. It could be reconsidered to have default affiliate (could be a minter by default).
DEV NOTE:
This is by design. Basically so that it's known that the contract is truly ownerless and community driven. When a default fee is set it implies
that the default recipient is a beneficiary with alterior motives of gain from commerce in the initial contract. Users should be the ones responsible
for the markets and it's management. Affiliates were allowed because users themselves asked for a way to promote their own products.
Known attacks in context of the smart contracts
Integer Overflow and Underflow
It was a general recommendation for the usage of SafeMath prior to the release of Solidity 0.8. Because the contract relys on the ^0.8 version it is generally not needed (may note the exception of the usage of ** operation which is not relevant to the context and the logic of the audited contract).
Timestamp Dependence
The block.timestamp is widely used in the contract for hashing and timelimits definitions. Though because the usage is based on the timelimits set for the contracts checkpoints in time which are generally working with long durations the attacks based on the block timestamps manipulations are not relevant to exploit the logics. TODO: if there checks needed on contract creations to prevent the frontend errors and the time maniputation? TODO: if some preventions to avoid the mempool long processing times.
Reetrancy Attack
There are no space for reetrancy attack approach. The deployment is safe as no the other contracts are called except the erc20 contracts which are protected by lock .
Recommendations
Documentation
There is general recommendation to have better documentation for all contract methods.
Using latest compiler
The contract depends on versions ^0.8.0 . It is always recommended to explicitly set the latest version of the solidy compiler (latest 0.8.24 at the time of the audit) to prevent the contract compilation and deployment to the production using the previous version of the compiler.
Contract deployment size
Due to its size, the contract deployment completely depends on the solc optimizer covering the "compression" from 44493 to the bytesize of 23432. (a limit introduced in the EIP-170 update placed the bytesize contract limit to 24576). It is recommended not to heavily rely to the bytesize optimizer as its behavior and effectiveness is the subject of the changes in the future releases of solc. It can be explored the way of splitting the contract to the libaries to be deployed separately.
ERC20 EIP-2612 extension
There is recommendation to use EIP-2612 extension to simplify the user contract comminication to avoid ERC-20 approve required by transferFrom . Instead of approve which requires the msg.sender that implys only user to set allowance, the EIP-2612 replaces this to signatures which can obsolete the extra call to the token contract as a need of setting allowance by user. It is relevant for new tokens deployed after 2020 (setup of EIP), reference ERC20Permit in OpenZeppelin.
Test Suite Results
The project had no test coverage, though I recommend to always have the tests simulating the complete lifecicle of the contract involving all operations and also to have written tests cover both negative (tests when functions should fail) and positive (tests when functions should pass) cases together with the replays attempts of the every contract api endpoint.
For this contract the test suite is designed and written using Truffle Suite:
Compiling your contracts...
===========================
> Compiling ./contracts/DDE.sol
> Compiling ./contracts/ERC20.sol
> Compiling ./contracts/IERC20.sol
> Compiling ./contracts/TDAI.sol
> Compiling ./contracts/WETH.sol
> Artifacts written to /tmp/test--18928-pjEiu1C2p9r4
> Compiled successfully using:
- solc: 0.8.24+commit.e11b9ed9.Emscripten.clang DDE dsize: 23432
Contract: DDE
? pre-check-1 tdai balance 100M
? pre-check-2 defaults
? change-minter-1 (51ms)
? change-minter-2 (61ms)
? change-threshold-1 (55ms)
? deposit-1 (1020ms)
? deposit-2 (331ms)
? deposit-3 (607ms)
? withdraw-1 (104ms)
? withdraw-2 (104ms)
? withdraw-3 (101ms)
? createContract-1 (235ms)
? acceptOffer-1 (320ms)
? completeEscrow-1 (112ms)
? completeEscrow-2 (136ms)
? dde-one-go-1 (756ms)
? dde-one-go-2-cancel-1 (764ms)
? dde-one-go-2-cancel-2 (796ms)
? dde-one-go-with-remove-1 (1077ms)
? change-fee-1 (42ms)
? dde-one-go-with-fee-1 (745ms)
? collected-fee-1
? dde-one-go-with-update-quantity-1 (834ms)
? collected-fee-2
? dde-one-go-with-expire-1 (820ms)
? dde-one-go-with-tags-and-extension-1 (1275ms)
? dde-one-go-private-cancel-1 (715ms)
? dde-one-go-private-1 (1190ms)