search

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

1 stars 0 forks source link

withdrawERC20 does not work on non-standard compliant tokens like USDT. #299

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/bcae7d71718c8eb744e4465d68fd6efa2bde7b06/contracts/NibblVault.sol#L517

Vulnerability details

Vulnerability Details

the withdrawERC20 function use the standard IERC20 interface from openzeppelin, which expect a returns from the interface, this works for the most ERC20 token, however for USDT token that didnt follow the ERC20 standard didnt have a return value on the transfer function, if the basket contract hold any USDT token, the USDT token would be stuck because when the user call withdrawERC20 with asset set as USDT, this transaction would revert and the token is not transferred.

Mitigation

consider using safeERC20 from openzepelin, to handle odd ERC20 token

Impact

Permanent freezing for USDT token.

POC

https://github.com/code-423n4/2022-06-nibbl/blob/bcae7d71718c8eb744e4465d68fd6efa2bde7b06/contracts/NibblVault.sol#L517

mundhrakeshav commented 2 years ago

63

GalloDaSballo commented 2 years ago

Findings Contingent on a specific token are historically Medium at best

Notice that USDT will actually reserve as the fact that the call returns nothing means the compiler will revert

HardlyDifficult commented 2 years ago

Both the vault and basket are geared towards holding NFTs. Including ERC20 withdraw functions appears to be a safety measure to allow recovery of those assets if a mistake was made. I agree they should consider a change here, but since it does not impact the protocol's intended use case I'll downgrade this report to a Low risk and converting this into a QA report for the warden.