search

code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

QA Report #171

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1 Unused parameter

Impact efficiency code and reduce gas cost

Proof of Concept https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L309

Tool Used Remix

Recommended Mitigation Steps remove unnecessary code. i suggest to change

function moduleIssueHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {

to

function moduleIssueHook(ISetToken _setToken) external override onlyModule(_setToken) {

2 Missing comment of parameter

impact componentIssueHook and componentRedeemHook have natspec comment which is missing the _setTokenAmount and _isEquity function parameter. Issues with comments are low risk based on Code4rena risk categories.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L338

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L368

Tool Manual Review

Recommended Mitigation Steps Add natspec comments include _setTokenAmount and _isEquity parameter.

3 Typo

Impact Misleading the user

Proof of Concept https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L215

Tool Manual Review

Recommended Mitigation Steps Fix it from Manger to manager

4 Pragma version

Impact smart contract isn't work properly

Proof of Concept https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L19

Tool Remix

Recommended Mitigation Steps pragma version must be update to version 0.8.0.

ckoopmann commented 2 years ago

The unused parameter referenced in the first suggestion is part of the IModuleIssuanceHook interface, so we cannot remove it unfortunately.

ckoopmann commented 2 years ago

Regarding issue 4: An older pragma version does not render this contract unusable. In fact due to the version on the dependencies of the set protocol architecture we are forced to use this version and will not change it unless absolutely necessary to mitigate a high risk issue (which I haven't seen so far).

ckoopmann commented 2 years ago

Not very much actionable stuff in here other than the missing comments on the parameters and the wrong casing. I also don't think writing "manager" instead of "Manager" would really "mislead the user" as claimed in this report. Overall one of the lower quality QA reports I have seen so far. Not labelling this as "disputed" though since above mentioned issues are technically correct.