Set tokens will be updated incorrectly

[code423n4]

Lines of code

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

Vulnerability details

Medium

Set tokens will be updated incorrectly

Although the functions doesn't utilise the other returned arguments, it will still impact values used elsewhere.

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

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

For the above mentioned functions, their calculations is as a result of an erroneous calculation of the pre and post notional values,

The comment below stipulates:

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/lib/Position.sol#L255

but it uses the prePosition unit :

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/lib/Position.sol#L216

instead of the _postTotalNotional.

Also, the returned value of position unit can be zero if it's the first time the component is being added, this is so because the default position is the last thing that gets edited in calculateAndEditDefaultPosition() but the value is used prior to its modification :

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/lib/Position.sol#L109

Recommendation

As per the comment on line 255 of calculateDefaultEditPositionUnit(), use the postTotalNotional as the subtrahend.

[ckoopmann]

As far as I can see the function works as intended

The point of this function is to calculate the position unit after (post) the trade. Normally this would just be _postTotalNotional.preciseDiv(_setTokenSupply) and we wouldn't need the preTrade balance / units.

However a part of the notional (the total balance of the component token) might have been airdropped tokens (that have been airdropped long before the trade), which are not to be included in the set tokens components. To avoid these airdropped tokens being added to the component position we subtract the airdropped amount (which is the difference between the preTradeNotional and the total amount of the component the set should contain based on its component position).

I agree the comment is somewhat misleading, however this was part of the pre-existing and audited SetProtocol setup.

[gzeoneth]

Downgrading to QA due to confusing comment

[gzeoneth]

Consider with #210