Olympus DAO contest - mics's results

Table Of Content

• QA REPORT * Use safeApprove() instead approve() * Unused success return value * Use safeTransfer() instead transfer() * Missing two steps verification process * Wrong use of assert * Avoid floating pragma * Missing 0 address check at transfer * Array access is out of bounds * Should approve(0) first * Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons. * Consider adding constant variables instead of hardcoded strings * Add event to the following functions * Events not emitted for important state changes

QA REPORT

Use safeApprove() instead approve()

Use openzeppelin safeApprove() method instead of approve() in the following locations.

Code Instances:

• Operator.t.sol#L191
• Governance.t.sol#L95
• Operator.t.sol#L198
• Operator.t.sol#L193
• BondCallback.t.sol#L206
• BondCallback.t.sol#L208
• TRSRY.t.sol#L155
• Governance.t.sol#L101
• Governance.t.sol#L103

Unused success return value

The following calls ignores the return value of the called function that might indicate the the call failed.

Code Instances:

• Operator.t.sol#L328
• Operator.t.sol#L300
• VOTES.t.sol#L49
• Operator.t.sol#L1380
• Operator.t.sol#L342
• Operator.t.sol#L1319
• Operator.t.sol#L997
• Operator.t.sol#L1882
• Operator.t.sol#L314
• Operator.t.sol#L1240
• Operator.t.sol#L1222
• Operator.t.sol#L1299
• Operator.t.sol#L1915
• Operator.t.sol#L1866
• Operator.t.sol#L398
• Operator.t.sol#L1864
• Operator.t.sol#L820
• Operator.t.sol#L1154
• RANGE.t.sol#L122
• Operator.t.sol#L1202
• Heart.t.sol#L247
• RANGE.t.sol#L137
• Operator.t.sol#L1278
• Operator.t.sol#L1246
• Governance.sol#L258
• Operator.t.sol#L1069
• Operator.t.sol#L438
• Operator.t.sol#L1992
• Governance.sol#L311
• Operator.t.sol#L1404
• Operator.t.sol#L1111
• Operator.t.sol#L399
• Operator.t.sol#L927
• Operator.t.sol#L1916
• Operator.t.sol#L933

Use safeTransfer() instead transfer()

Use openzeppelin safeTransfer() method instead of transfer() in the following locations.

Code Instances:

• VOTES.t.sol#L73
• VOTES.t.sol#L34
• Governance.sol#L78
• VOTES.t.sol#L49

Missing two steps verification process

The process of transferring ownership is dangerous since typing the wrong address can lead to severe implications. It is better to have to steps verification process with set and claim functions to decrease the chances of human error. Consider changing to two steps verification process of transferring privileges. Human mistakes can happen.

For instance, Kernel.t.sol

Wrong use of assert

You should use if-revert or require statements instead of assertions in production.

Code Instances:

• BondCallback.t.sol#L476
• BondCallback.t.sol#L483

Avoid floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. (SWC-103)

Code Instances:

• BondCallback.t.sol
• VOTES.t.sol
• PriceConfig.t.sol
• RANGE.t.sol
• TreasuryCustodian.t.sol
• PRICE.t.sol

Missing 0 address check at transfer

Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.

Code Instances:

• VOTES.sol#L60
• Operator.sol#L329
• TRSRY.sol#L98
• TRSRY.sol#L81

Array access is out of bounds

There is no check for the access to be in the array bounds.

Code Instances:

• Kernel.sol#L325
• RANGE.sol#L81
• PRICE.sol#L183

Should approve(0) first

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code Instances:

• TRSRY.t.sol#L130
• BondCallback.t.sol#L213
• Operator.sol#L166
• Operator.t.sol#L193
• TRSRY.t.sol#L100
• Operator.t.sol#L196
• Governance.t.sol#L95
• BondCallback.t.sol#L211
• Governance.t.sol#L97
• BondCallback.t.sol#L476
• Operator.t.sol#L198
• TRSRY.t.sol#L132

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Code Instances:

• BondCallback.t.sol#L249
• VOTES.sol#L27
• PRICE.sol#L113
• BondCallback.sol#L178

Consider adding constant variables instead of hardcoded strings

A good practice is to use constant variables instead of hardcoded strings in the code.

Code Instances:

• Heart.t.sol#L259
• Governance.t.sol#L156
• Kernel.t.sol#L165
• Operator.t.sol#L407
• Operator.t.sol#L172
• Operator.sol#L157
• PriceConfig.sol#L19
• PRICE.t.sol#L37
• Kernel.t.sol#L341
• TreasuryCustodian.sol#L28
• PRICE.sol#L108
• Operator.t.sol#L1727
• PRICE.t.sol#L485
• Deploy.sol#L270
• TRSRY.t.sol#L49
• RANGE.t.sol#L48
• Operator.t.sol#L1954
• RANGE.sol#L110
• Kernel.t.sol#L154
• MINTR.sol#L20
• Kernel.t.sol#L140
• Kernel.t.sol#L86
• Heart.t.sol#L162
• VoterRegistration.t.sol#L55
• BondCallback.t.sol#L177
• Operator.sol#L158
• Kernel.t.sol#L212
• BondCallback.sol#L49
• BondCallback.t.sol#L105
• Operator.t.sol#L165
• Kernel.t.sol#L129
• Operator.t.sol#L1537
• BondCallback.sol#L50
• Kernel.t.sol#L110
• Kernel.t.sol#L149
• VoterRegistration.t.sol#L47
• BondCallback.t.sol#L183
• Heart.sol#L70
• BondCallback.t.sol#L488
• Governance.t.sol#L138
• Heart.t.sol#L300
• TreasuryCustodian.t.sol#L54
• Heart.t.sol#L74
• Heart.t.sol#L121
• Operator.t.sol#L2081
• BondCallback.t.sol#L184
• BondCallback.t.sol#L178
• Heart.t.sol#L120
• Kernel.t.sol#L76
• Kernel.t.sol#L216
• Kernel.t.sol#L358
• Kernel.t.sol#L88
• PRICE.t.sol#L155
• Kernel.t.sol#L78
• Operator.t.sol#L1502
• Operator.t.sol#L1686
• RANGE.t.sol#L49
• Governance.sol#L62
• Kernel.t.sol#L243
• Kernel.t.sol#L168
• Operator.t.sol#L2005
• Deploy.sol#L268
• Operator.t.sol#L1475
• Operator.t.sol#L446
• Kernel.t.sol#L53
• Kernel.t.sol#L356
• Kernel.t.sol#L95
• BondCallback.t.sol#L179
• Operator.t.sol#L173
• Kernel.t.sol#L139
• Kernel.t.sol#L103
• Operator.t.sol#L1794
• Kernel.t.sol#L68
• Kernel.t.sol#L80
• Kernel.t.sol#L232
• BondCallback.t.sol#L429
• Kernel.t.sol#L280
• Operator.sol#L156

Add event to the following functions

Code Instances:

• VoterRegistration.sol#L19
• Governance.t.sol#L46
• RANGE.t.sol#L442
• Governance.sol#L61
• Kernel.t.sol#L135
• Operator.sol#L154
• Kernel.t.sol#L108
• PRICE.sol#L208
• Kernel.t.sol#L309
• Kernel.t.sol#L23
• PRICE.t.sol#L33
• PRICE.sol#L183
• RANGE.t.sol#L35
• BondCallback.sol#L42
• RANGE.t.sol#L424
• RANGE.t.sol#L453
• Kernel.t.sol#L94
• PriceConfig.t.sol#L37
• Heart.sol#L69
• RANGE.t.sol#L433
• Operator.sol#L598
• PriceConfig.sol#L18
• Kernel.t.sol#L326
• RANGE.t.sol#L471
• Deploy.sol#L264
• Kernel.t.sol#L270
• Kernel.t.sol#L62

Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Code Instances:

• RANGE.t.sol#L433
• TreasuryCustodian.sol#L27
• PriceConfig.t.sol#L37
• Heart.sol#L135
• BondCallback.sol#L42
• RANGE.t.sol#L35
• Kernel.t.sol#L23
• Heart.sol#L130
• Kernel.t.sol#L270
• VOTES.t.sol#L23
• RANGE.t.sol#L424
• Deploy.sol#L264
• TreasuryCustodian.t.sol#L26
• Operator.sol#L154
• PRICE.sol#L208
• Kernel.sol#L217
• Kernel.t.sol#L94
• MINTR.sol#L15
• RANGE.t.sol#L462
• Operator.sol#L589
• TRSRY.t.sol#L28
• Kernel.t.sol#L326
• Kernel.t.sol#L309
• PRICE.t.sol#L33
• Operator.sol#L598
• BondCallback.t.sol#L80
• Kernel.t.sol#L209