search

code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

FungibleAssetVaultForDAO.withdraw calls native token payable.transfer, which is unsafe for smart contracts #211

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L201

Vulnerability details

Impact

If a native token is used as vault collateral, it is sent out via payable.transfer call. This is unsafe as transfer has hard coded gas budget and can fail when the whitelisted msg.sender is a smart contract. Such transactions will fail for smart contract callers which don't fit to 2300 gas stipend transfer have.

As withdraw call is the only way to retrieve collateral funds, this means that such accounts will be unable to retrieve collateral at all.

Setting severity to medium as these funds are shared between the whitelisted addresses, but the contract functionality availability is violated.

Proof of Concept

FungibleAssetVaultForDAO.withdraw performs payable(msg.sender).transfer(amount) call for collateral retrieval:

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L201

Whenever msg.sender is an another contract which either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on native token transfer exceeds 2300 gas consumption limit the native tokens sent will be undelivered. This leads to systematic fail of withdraw and the collateral retrieval functionality in such cases.

References

The issues with transfer() are outlined here:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

As withdraw is nonReentrant, the reentrancy isn't an issue and transfer() can be replaced.

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

spaghettieth commented 2 years ago

Duplicate of #39

dmvt commented 2 years ago

See comment on #39. This is a QA issue.