search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

QA Report #137

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

We list 1 low-critical finding and 2 non-critical findings:

(Low) ZcToken will get wrong values due to the wrong maturityRate

Impact

ZcToken will get wrong values if users don't call matureMarket after maturity.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/ZcToken.sol

In ZcToken.convertToUnderlying, ZcToken.previewRedeem and ZcToken.maxWithdraw, these functions will be reverted after maturity due to division by 0 if market is not matured.

In ZcToken.convertToPrincipal and ZcToken.previewWithdraw, these functions return incorrect value if they are called after maturity but the market is not matured. This will cause ZcToken.withdraw to pass incorrect value to authRedeem, and potentially wastes user gas.

Recommended Mitigation Steps

Check the market should be matured.

(Non) Swivel.setFee should check length <= feeNominator array length

Impact

It will be reverted if length > feeNominator array length.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L495

In setFee, the length of array i and d should <= 4 which is the feeNominator array length.

Recommended Mitigation Steps

Check i and d length <= feeNominator array length.

(Non) Using ecrecover is against best practice

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/Erc20.sol#L141

Recommended Mitigation Steps

Take these implementation into consideration

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/utils/cryptography/draft-EIP712.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

robrobbins commented 2 years ago

i don't feel that ecrecover is a liability. OZs ECDSA is a little bit of a solution looking for a problem in this regard imo