Platform: Code4rena
Start Date: 26/08/2021
Pot Size: $200,000 USDC
Total HM: 17
Participants: 11
Period: 14 days
Judge: ghoulsol
Total Solo HM: 12
Id: 23
League: ETH
Rank: 4/11
Findings: 4
Award: $13,992.72
🌟 Selected for report: 6
🚀 Solo Findings: 0
1557.1694 NOTE - $1,557.17
4671.5083 USDC - $4,671.51
pauliax
Anyone can call function notionalCallback with arbitrary params and pass the auth check. The only auth check can be easily bypassed by setting sender param to the address of this contract. It allows to choose any parameter that I want: function notionalCallback( address sender, address account, bytes calldata callbackData ) external returns (uint256) { require(sender == address(this), "Unauthorized callback");
It needs to check that msg.sender is Notional.
#0 - jeffywu
2021-09-11T17:43:17Z
Duplicate of #71
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
pauliax
function transferFrom in nTokenERC20Proxy emits Approval event: emit Approval(msg.sender, from, newAllowance); The order of the parameters is wrong, 'msg.sender' and 'from' should be in the opposite order. This may confuse frontends or other services that consume these events from the outside.
emit Approval(from, msg.sender, newAllowance);
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
pauliax
function activateNotional calls Address.isContract(...) but does not check the returned value, thus making this call pretty much useless: Address.isContract(address(notionalProxy_));
Wrap this in a require statement.
🌟 Selected for report: pauliax
346.0377 NOTE - $346.04
1038.113 USDC - $1,038.11
pauliax
function notionalCallback (in NotionalV1ToNotionalV2 and CompoundToNotionalV2) declares to return uint, however, no actual value is returned.
Either remove the return declaration or return the intended value (I assume it may return a value that it gets from depositUnderlyingToken/depositAssetToken). Otherwise, it may confuse other protocols that later may want to integrate with you.
🌟 Selected for report: pauliax
346.0377 NOTE - $346.04
1038.113 USDC - $1,038.11
pauliax
contract NotionalV1ToNotionalV2 has an empty receive function which allows it to receive Ether. I suppose this was needed to receive ETH when withdrawing from WETH. As there is no way to send out accidentally sent ETH from this contract, I suggest adding an auth check to this receive function to only accept ETH from WETH contract.
require(msg.sender == address(WETH), "Not WETH");
🌟 Selected for report: pauliax
312.5 NOTE - $312.50
937.5 USDC - $937.50
pauliax
Unused variables, e.g.: int256 internal constant ETH_DECIMAL_PLACES = 18; remove it to save some gas or use it where intended.
#0 - jeffywu
2021-09-11T15:07:42Z
Unused constants do not have an impact on deployed code.
#1 - ghoul-sol
2021-09-15T23:01:14Z
Technically, it makes deployment more expensive. Keeping as is.
🌟 Selected for report: pauliax
312.5 NOTE - $312.50
937.5 USDC - $937.50
pauliax
There are several checks that uint is not negative, e.g.: cashGroup.maxMarketIndex >= 0 && These checks are pretty much useless as uint can never be negative. Remove them to save some gas.
#0 - jeffywu
2021-09-11T15:07:11Z
Again, pretty generic but fair point. It's not like I'm unaware of this but some things do creep in in a large code base. Other wardens have provided better issue descriptions that I think we should reward more.
🌟 Selected for report: pauliax
312.5 NOTE - $312.50
937.5 USDC - $937.50
pauliax
There are values that are accessed multiple times in the same function, e.g. setCashGroupStorage queries cashGroup.maxMarketIndex 7 times. It would save some gas if you cache such values in temporal variables and re-use them where necessary.
#0 - jeffywu
2021-09-11T15:05:58Z
This is a pretty generic comment, although a fair point.