Nibbl contest - Nyamcil's results

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

https://github.com/code-423n4/2021-04-meebits-findings/issues/2 https://github.com/code-423n4/2021-10-tally-findings/issues/20 https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

Recommended Mitigation Steps

Use call() instead of transfer().

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc. depending on the data type). Explicitly initializing it wastes gas.

I would recommend leaving the initialized variables with the default values on the following lines:

nibbl-smartcontracts/contracts/Basket.sol:43:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:70:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:93:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:506:         for (uint256 i = 0; i < _assetAddresses.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:525:         for (uint256 i = 0; i < _assets.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:547:         for (uint256 i = 0; i < _assets.length; i++) {

Prefix Increments cost less gas than postfix increments ++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

I would changing the increment from postfix to prefix in the following lines:

nibbl-smartcontracts/contracts/Basket.sol:43:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:70:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:93:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:506:         for (uint256 i = 0; i < _assetAddresses.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:525:         for (uint256 i = 0; i < _assets.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:547:         for (uint256 i = 0; i < _assets.length; i++) {

Increments Can be Unchecked In Solidity 0.8+, there’s a default overflow check on unsigned integers. Unchecking this saves gas each iteration but sacrifices code readability. Gas optimizing solutions can be found at https://github.com/ethereum/solidity/issues/10695

I would recommend that the following lines be unchecked to optimize gas costs

nibbl-smartcontracts/contracts/Basket.sol:43:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:70:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/Basket.sol:93:       for (uint256 i = 0; i < _tokens.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:506:         for (uint256 i = 0; i < _assetAddresses.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:525:         for (uint256 i = 0; i < _assets.length; i++) {
nibbl-smartcontracts/contracts/NibblVault.sol:547:         for (uint256 i = 0; i < _assets.length; i++) {