A malicious _cBridgeData.token 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(_cBridgeData.token), ...) is called on potentially malicious _cBridgeData.token. 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 _cBridgeData.token, which is pretty much next to useless considering we already have full control over _cBridgeData.token. 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 _cBridgeData.token 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
Lines of code
https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L57 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L92 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L159
Vulnerability details
Impact
A malicious
_cBridgeData.tokencan 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(_cBridgeData.token), ...)is called on potentially malicious_cBridgeData.token. Sinceapprovewill be called withcallopcode, it is possible to utilize this chance to re-enter facet.We haven't identified any potential attack vectors except withdrawing previously transferred
_cBridgeData.token, which is pretty much next to useless considering we already have full control over_cBridgeData.token. 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
_cBridgeData.tokenif possible. If this is not desired due to reduced flexibility of facet, addnonReentrantmodifier to public/external functions instead.