search

manifoldfinance / mevETH2

mevETH LST Protocol - Repo has migrated see link
https://github.com/MEV-Protocol/meveth
27 stars 2 forks source link

Audit: MANETH-32 #135

Closed sandybradley closed 1 year ago

sandybradley commented 1 year ago

DISCREPANCIES BETWEEN OFT CROSS-CHAIN/SAME-CHAIN TRANSFERS

SEVERITY: Low

PATH: OFTWithFee.sol:_transfer:L100-105

https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/layerZero/oft/OFTWithFee.sol#L100-L105

REMEDIATION:

the mentioned variables should be marked as private instead of public

DESCRIPTION

The OFTWithFee.sol contract has differences from LZ implementation https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts /token/oft/v2/fee/OFTWithFee.sol. The original contract implements OZ’s ERC20 contract which uses _transfer, but this contract implements Solmate’s ERC20 which doesn’t. This contract also defines a new internal function _transfer() (L100-L105) and as stated by the doc string it might implement token fees, slashing, etc if needed. This one is called from OFTWithFee.sol:_transferFrom() only and thus used when OFT is transferred between chains. On the other hand, OFTWithFee.sol is inherited from solmate/tokens/ERC20.sol that has the implementation of transfer() which doesn’t use _transfer(). In such case, if fees/slashing/etc mechanisms are added to _transfer() there will be discrepancies in how token behaves when transferring in the same chain versus cross-chain.

sandybradley commented 1 year ago

Not sure I follow this logic.

_transfer emulates the OpenZeppelin version (except for address(0) check, which I can add for safety)