Call() Should Be Used Instead of Transfer()

[code423n4]

Lines of code

https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L517
https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L526
https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/Basket.sol#L80
https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/Basket.sol#L87
https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/Basket.sol#L94

Vulnerability details

Impact

The use of the deprecated transfer function will likely cause failure in the future. The receiver has multiple ways in which to make the transfer fail.

1. Not having a payable function
2. Having a payable function that uses more than 2300 gas
3. Having a payable function under 2300 gas that is called by proxy causing it to be over 2300 gas.
4. There may be a multisignature wallet that requires over 2300 gas.

Impacted Lines:

contracts/NibblVault.sol#L517:    IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
contracts/NibblVault.sol#L526     IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));
contracts/Basket.sol#L80     _to.transfer(address(this).balance);
contracts/Basket.sol#L87     IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));
contracts/Basket.sol#L94     IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));

Proof of Concept

code-423n4/2021-04-meebits-findings#2
code-423n4/2021-10-tally-findings#20
code-423n4/2022-01-openleverage-findings#75

Recommended Mitigation Steps

Use call() instead of transfer().

[mundhrakeshav]

Those are ERC20 contracts

[HardlyDifficult]

Some of the examples were ERC20, which do not apply here. But an ETH transfer was included as well.

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 converting this into a QA report for the warden.