search

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

6 stars 4 forks source link

NXTPFacet does not check whether _nxtpData.invariantData.sendingAssetId is benign, resulting in reentrancy problems #136

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L180

Vulnerability details

Impact

A malicious _nxtpData.invariantData.sendingAssetId can be crafted to re-enter facet. While no immediate attack vectors are found, preventive measures are suggested to fully close off such attack chances.

Proof of Concept

In _startBridge, LibAsset.approveERC20(IERC20(sendingAssetId), ...) is called on potentially malicious _nxtpData.invariantData.sendingAssetId. Since approve will be called with call opcode, it is possible to utilize this chance to re-enter facet.

We haven't identified any potential attack vectors except withdrawing previously transferred _nxtpData.invariantData.sendingAssetId, which is pretty much next to useless considering we already have full control over _nxtpData.invariantData.sendingAssetId. But generally, reducing the attack surface, especially for something as dangerous as reentrancy is a good thing to do.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Validate _nxtpData.invariantData.sendingAssetId if possible. If this is not desired due to reduced flexibility of facet, add nonReentrant modifier to public/external functions instead.

H3xept commented 2 years ago

Duplicate of #109

gzeoneth commented 2 years ago

Duplicate report by the same warden #134