search

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

1 stars 1 forks source link

QA Report #104

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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

// Use a safe downcast function e.g. wfCashLogic::_safeUint88
function _safeUint88(uint256 x) internal pure returns (uint88) {hil
    require(x <= uint256(type(uint88).max));
    return uint88(x);
}

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:

int256 underlyingExternal = NotionalV2.convertCashBalanceToExternal(currencyId, cashBalance, true);
require(underlyingExternal > 0, "Must Settle");
int256 pvExternal = (pvInternal * precision) / Constants.INTERNAL_TOKEN_PRECISION;
// PV should always be >= 0 since we are lending
require(pvExternal >= 0);

Recommended Mitigation Steps

Consider returning 0 if the condition is not met.

if (underlyingExternal < 0) return 0;

if (pvExternal < 0) return 0;

As a consequence, because totalAssets() can now possibly return 0, convertToShares() has to be modified to check that totalAssets() is non-zero instead of totalSupply() to ensure compatibility with the specification: “MUST NOT revert unless due to integer overflow caused by an unreasonably large input.”

function convertToShares(uint256 assets) public view override returns (uint256 shares) {
  uint256 totalAssets = totalAssets();
  if (totalAssets == 0) {
    // Scales assets by the value of a single unit of fCash
    uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
    return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
  }
  return (assets * totalSupply()) / totalAssets;
}

L03: _redeemMaturedPositions() might run out of gas if there are too many matured positions to be redeemed at once

Line 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.

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:

  1. The underlying / asset token requires allowance to be set to 0 first before approve can be called again (Eg. USDT)
  2. There is excess allowance.

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

token.safeTransferFrom(msg.sender, address(this), depositAmountExternal);

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 for fCashAmount. For example, if 1 DAI = 1 fCash = 1 wfCash and the contract has 900 DAI stuck, a user can specify 0 for depositAmountExternal. The action variable will still be encoded correctly because depositAmountExternal 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 .

ckoopmann commented 2 years ago

Silent overflow of _fCashAmount - Good catch, will have to review and probably mitigate this. This might even be more than just a "QA" issue in my view.

Short but pretty good report with interesting findings some of which other wardens have or probably would have raised as standalone issues.