Audit fixes #167
Audit fixes #167
Conversation
| uint256 extensionValue; | ||
| if (msg.value != valueRequired(tokenAddress, amount, nativeFee)) { | ||
| revert InvalidValue(); | ||
| } |
There was a problem hiding this comment.
I was mostly thinking about just checking in the original initTransferExtension that extensionValue = 0. But overall, I think this works too.
Although this results in an extra call to Wormhole, even though that check is already performed internally.
There was a problem hiding this comment.
Wait, what about regular fee? I thought that we need to check it as well, but valueRequired doesn't consider it. Also, for Sepolia chains messageFee is 0, but for other chains user might pay for it and then msg.value would be greater than a value calculated by valueRequired
There was a problem hiding this comment.
fee refers to tokens, never Ether. valueRequired is a virtual function, and it’s calculated correctly.
There was a problem hiding this comment.
Ah so for wormhole it is already checked when sending a message? Then I'd probably just check for Ethereum version like you say
There was a problem hiding this comment.
Let's keep this perhaps in order not to rely on wormhole
There was a problem hiding this comment.
Adding checks for value == 0 in the default initTransferExtension should be enough
| uint256 extensionValue; | ||
| if (msg.value != valueRequired(tokenAddress, amount, nativeFee)) { | ||
| revert InvalidValue(); | ||
| } |
There was a problem hiding this comment.
Adding checks for value == 0 in the default initTransferExtension should be enough
https://github.com/AuditOneAuditReviews/Omnibridge2-audit-review/issues/3
https://github.com/AuditOneAuditReviews/Omnibridge2-audit-review/issues/24
https://github.com/AuditOneAuditReviews/Omnibridge2-audit-review/issues/10
https://github.com/AuditOneAuditReviews/Omnibridge2-audit-review/issues/19
Plus, validating
msg.valueon the EVM side