search

code-423n4 / 2022-06-nibbl-findings

1 stars 0 forks source link

Using `transfer` for native token do not work when recipient is smart contract #308

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L80

Vulnerability details

Impact

Function Basket.withdrawETH() uses transfer function to send ETH to address _to.

This is unsafe as transfer has hard coded gas budget (2300 gas) and can fail when the user is a smart contract.

Proof-of-concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L80

Tools Used

Manual code review

Recommended Mitigation Steps

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

mundhrakeshav commented 2 years ago

18

HardlyDifficult commented 2 years ago

Agree that using .transfer is now discouraged. I think a difference here as compared to other contests is that the _to address is simply an input to this function call -- so if it reverts they could try again with a EOA and then transfer manually to the contract. Lowering risk and merging with the warden's QA report #310