QA Report #104
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Codebase Impressions & Summary
The relevant codebase to be reviewed is very well structured, commented and written. This makes it easier to understand what is going on. That said, however, because this is an integration between 2 protocols, the hidden scope (of understanding how these 2 protocols work) can be quite substantial and daunting for anyone who is not familiar with these 2 platforms.
Tracing function calls can also get quite complex since there are many contracts to jump between. This is made worse by the incomplete technical documentation (especially on Notional’s side), so some effort should be spent by the Notional team to improve this. The technical videos may help to supplement this gap, but should not be a replacement for it.
Both unit and integration tests were written extensively for the contracts to ensure they work as intended. Credit to the team for having a comprehensive test suite!
Low Severity Findings
L01: Silent overflow of
_fCashAmount
Line References
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L526
Description
If a
_fCashAmount
value that is greater than uint88 is passed into the_mint
function, downcasting it to uint88 will silently overflow.Recommended Mitigation Steps
L02: Incompatibility with ERC-4626
Line References
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L23
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L42
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L48
Description
The EIP-4626 specification requires that
totalAssets()
to NOT revert, but the current implementation does so in the underlying methods:Recommended Mitigation Steps
Consider returning 0 if the condition is not met.
As a consequence, because
totalAssets()
can now possibly return 0,convertToShares()
has to be modified to check thattotalAssets()
is non-zero instead oftotalSupply()
to ensure compatibility with the specification: “MUST NOT revert unless due to integer overflow caused by an unreasonably large input.”L03:
_redeemMaturedPositions()
might run out of gas if there are too many matured positions to be redeemed at onceLine References
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L385-L410
Description
A set token can consist of a mixture of wrapped fCash and non-wrapped fCash positions. Hence, the combination of iterations through all positions, together with the need to redeem all mature positions, could cause the transaction to revert from insufficient gas due to the prevalent gas limits of a transaction.
We note that an account can have either of 2 portfolio types: an array portfolio (hold up to 7 fCash assets) or a bitmap portfolio (up to 20 fCash assets), making this situation occurrence unlikely, but nevertheless, a possibility.
Recommended Mitigation Steps
Create a method that allows anyone to redeem individual matured positions.
Non-Critical Findings
NC01: Spelling and Grammar Fixes
Recommended Mitigation Steps
Ctrl + shift + f to find all, look for the following words (left) and rename it (right) accordingly.
MANGER
→MANAGER
does not require and additional redeem...
→does not require an additional redeem...
Alo adjust...
→Also adjust...
wether or not...
→whether or not..
the components of the fCash idd
→the components of the fCash id
NC02: Token approvals without first setting to 0
Line References
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L501-L505
Description
Approve is called again without setting to 0. This might be an issue but only if 2 conditions are true:
The first is not an issue for the existing nTokens (ETH, DAI, USDC, WBTC) but may not necessarily stay that way as the protocol grows and adds more assets. The second is unlikely to be an issue as well because the allowance SHOULD be entirely consumed:
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L83-L84
Recommended Mitigation Steps
None. Purely informational finding.
NC03: Possible to “steal” stuck tokens when minting wrapped fCash
Line References
https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L84-L94
Description
The wfCash contract does not check how many tokens are transferred via
token.safeTransferFrom(msg.sender, address(*this* ), depositAmountExternal);
. This makes it possible for a user to “steal” stuck funds by specifying a larger than expected value forfCashAmount
. For example, if 1 DAI = 1 fCash = 1 wfCash and the contract has 900 DAI stuck, a user can specify 0 fordepositAmountExternal
. Theaction
variable will still be encoded correctly becausedepositAmountExternal
is not used in the encoding.Recommended Mitigation Steps
Check token balances before and after transfer and calculate that the
fCashAmount <= tokens transferred * implied rate
.The text was updated successfully, but these errors were encountered: