search

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

1 stars 0 forks source link

unchecked return value from the transfer of erc20 token #249

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#L87 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L94 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L517

Vulnerability details

Impact

transfer() does not check the return , due which transfer may get failed without reverting

Proof of Concept

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

Tools Used

manual review

Recommended Mitigation Steps

verify the return value from the transfer

mundhrakeshav commented 2 years ago

74

HardlyDifficult commented 2 years ago

For the calls listed here, if a transfer fails the user could make the necessary corrections and try again. So checking the return value seems like a nice to have in order to better communicate errors to the user. Lowing risk and merging with the warden's QA report #245