LayerZero-Labs / LayerZero-v2

Other
492 stars 312 forks source link

Bug Report : Incorrect decision for invalid expiry - conflict between Docs and code #79

Closed 0xreadyplayer1 closed 4 days ago

0xreadyplayer1 commented 2 weeks ago

Bug Report

Note :- This is a low severity bug report due to high likelihood but low impact. i wanted to submit it through immunefi but the asset was not in-scope . Additionally i was not able to disclose using some security team email of LZ because it does not exist.

So i believe sharing this bug here is a good thing because it does not give an edge to any attacker. The layer zero labs might review the validity of the bug and decide to reward the whitehat as per their assesment.

This might not be your "Typical Critical" bug that puts funds at risk but this bug , if not mitigated can put future protocol developments and integrations at risk that assumes that code is implemented according to docs 100%.

Report

Vulnerability Details

The Layer-zero v2 docs states following about the invalidExpiry error

image

it states

When setting expiry time for the default library, thrown if the expiry time is set before the block timestamp.

This translates to

expiry < block.timestamp

or

expiry < block.number

it means the error InvalidExpiry(uint256 expiry, uint256 minExpiry) should not be thrown if

expiry >=block.number

however in the current implementation of setDefaultReceiveLibraryTimeout inside MessageLibManager

https://github.com/LayerZero-Labs/LayerZero-v2/blob/417cbb9eb68a4f678490d18728973c8c99f3f017/packages/layerzero-v2/evm/protocol/contracts/MessageLibManager.sol#L210

            if (_expiry <= block.number) revert Errors.LZ_InvalidExpiry();

The function reverts with this error even if expiry == block.number

which is not intended behaviour according to Docs.

Impact

While it does not have direct funds loss , which is why its low severity , it can cause future protocol implementations and integrations to be at risk because they would assume that expiry can be set for current block.number , which is correct according to Documentation . However the code will revert , reverting any crucial operations of the layer zero protocol components itself or the protocols being integrated.

Recommendation

Only revert if expiry < block.number otherwise update the documentation

0xreadyplayer1 commented 2 weeks ago

@TRileySchwarz @zouguangxian Mentioning you guys to have a look

0xreadyplayer1 commented 5 days ago

The target documentation for error codes can be found here

https://docs.layerzero.network/v2/developers/evm/troubleshooting/error-messages

InvalidExpiry(uint256 expiry, uint256 minExpiry)

When setting expiry time for the default library, thrown if the expiry time is set before the block timestamp.

@DanL0 Looking forward.

St0rmBr3w commented 4 days ago

@0xreadyplayer1 this is an inconsistency in the docs, will update accordingly to reflect the correct in-code effects.

0xreadyplayer1 commented 4 days ago

@St0rmBr3w but it can serve as a low severity issue because of the impact

contract fails to deliver promised returns but does not lose value

As The function reverts with this error even if expiry == block.number

which is not intended behaviour according to Docs.

So according to the rules of the bug bounty programs , it definiteky serve as a low severity issue to be paid : )

St0rmBr3w commented 2 days ago

@0xreadyplayer1 you can submit the request through to Immunefi, but this is a nomenclature error in how the docs explain the correct code behaviour, rather than an actual bug report related to the nature of the code logic.

0xreadyplayer1 commented 12 hours ago

@St0rmBr3w immunefi does not have this category of bug to be submitted so i thought its better to submit here. However , i leave it on layer zero team to decide the fate of this report .

Thanks for the response