search

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

1 stars 0 forks source link

## Unsafe usage of Transfer instead of call to move ether #272

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

Basket.withdrawETH uses transfer to move Ether, transfer uses a limited gas stipend which can run out, making it impossible to move the ETH out.

From my experience a direct call via Gnosis Safe will still be fine, however a contract that implements any Storage related logic in receive will most likely revert.

Recommended Mitigation Steps

Refactor to the pattern you've already used of (bool _success, ) = payable(_to).call{value: address(this).balance}(""); require(_success);

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 #283