code-423n4 / 2022-03-biconomy-findings

0 stars 0 forks source link

LPToken.sol has inherited shadowed state variable update #18

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L39

Vulnerability details

Impact

In LPToken.sol the initialize() function passes the _trustedForwarder argument which then calls __ERC2771Context_init(_trustedForwarder) in an effort to set the passed in variable as the _trustedForwarder in the ERC2771ContextUpgradeable contract. The problem is that there is a shadowed state variable likely because the _trustedForwarder argument passed to the initialize() function in LPToken.sol has the same name as the _trustedForwarder storage variable in the inherited ERC2771ContextUpgradeable.sol. This can cause inheritance issues with state not being properly updated as intended.

Proof of Concept

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L39

Tools Used

Manual code review

Recommended Mitigation Steps

Consider renaming the _trustedForwarder variable to something else (like trustedForwarder) in order not to name clash with the _trustedForward variable in the inherited ERC2771ContextUpgradeable contract.

pauliax commented 2 years ago

_trustedForwarder is private in ERC2771ContextUpgradeable, so it can't clash: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/metatx/ERC2771ContextUpgradeable.sol#L14