Synthetixio / synthetix-v3

MIT License
114 stars 56 forks source link

Moss audit fixes #2190

Closed moss-eth closed 4 weeks ago

moss-eth commented 2 months ago

Informal audit report found below. Some of these issues were already fixed on the gov v3 branch, so all should be present on this branch, but not all will be found in the diff.

Other changes include:

report:

  1. The tweakEpochSchedule() function needs to be marked payable as the transmit function requires payment for the cross-chain broadcast. (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModule.sol#L150)
  2. The evaluate() function needs to be marked payable as the transmit function requires payment for the cross-chain broadcast. (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModule.sol#L379)
  3. The dismissMembers() function is incorrectly passing through msg.valueas the value to be sent cross-chain. No value should be transferred, therefore a value of 0 should be used. (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModule.sol#L242)
  4. The cast() function is incorrectly passing through msg.valueas the value to be sent cross-chain. No value should be transferred, therefore a value of 0 should be used. (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModuleSatellite.sol#L117)
  5. The withdrawVote() function is incorrectly passing through msg.valueas the value to be sent cross-chain. No value should be transferred, therefore a value of 0 should be used. (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModuleSatellite.sol#L122)
  6. The dismissMembers() function currently only transmits to the mothership chain. However, originally the function was intended to be called on the mothership chain and then transmit to all the other chains to notify that members had been dismissed. Instead of transmitting to the mothership chain, the message should be broadcasted to all supported chains (https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModule.sol#L231)
  7. The Wormhole receiver interface is incorrect and should implement a different function signature. (incorrect: https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/utils/core-modules/contracts/interfaces/IWormholeReceiver.sol#L44, correct: https://github.com/wormhole-foundation/wormhole/blob/d146f82bcacdbba03b00b1f74abae694f07dcafa/relayer/ethereum/contracts/interfaces/relayer/IWormholeReceiver.sol#L44)
  8. As it is necessary to transmit to all supported networks several times throughout the codebase, an additional broadcast function should be added to WormholeCrossChainModule.sol to handle such a case and reduce code repetition. Furthermore, a special case for transmitting to the chain itself is not necessarily (e.g. tweakEpochSchedule https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/protocol/governance/contracts/modules/core/ElectionModule.sol#L173), as transmit will simply perform an external call into itself on the local network.
  9. Two forms of refunds need to be added in transmit regarding the comment on https://github.com/Synthetixio/synthetix-v3/blob/15ddce226263eb990a20da1f1f32ab54d55258a3/utils/core-modules/contracts/modules/WormholeCrossChainModule.sol#L101. The first is if the cost variable exceeded the gas cost on the target chain, which can be refunded by specifying the refund address and chain per https://github.com/wormhole-foundation/wormhole/blob/d146f82bcacdbba03b00b1f74abae694f07dcafa/relayer/ethereum/contracts/interfaces/relayer/IWormholeRelayer.sol#L114. The second refund that needs to be done is for msg.value - cost - receiverValue, which should be refunded to the function caller in the event that msg.value exceeds the amount required by the function. If a broadcast function is implemented, special care should be taken to ensure that the total value across all chains is considered during the refund.