VIS-2 / taobank-04-24

0 stars 0 forks source link

Low Risk Issues #5

Closed 0xMilenov closed 4 months ago

0xMilenov commented 5 months ago
Issue Instances Gas
[L-01] Arrays can grow in size without a way to shrink them 2 None
[L-02] The decimals() function isn't included in the ERC-20 specification 2 None
[L-03] safeApprove() is deprecated 4 None
[L-04] Use Ownable2Step.acceptOwnership() instead of Ownable.transferOwnership 14 None
[L-05] Note that symbol() is not included in the ERC-20 standard 1 None
[L-06] ERC20 tokens with more than 18 decimals cause reverts 1 None

[L-01] Arrays can grow in size without a way to shrink them

As these arrays cannot shrink, if the array has a maximum size, it won't be possible to change its elements once it reaches that size. Otherwise, it can grow indefinitely in size, which can increase the likelihood of out-of-gas errors.

File: contracts/StabilityPool.sol

}515:         .push() = tokenToS;

515

[L-02] The decimals() function isn't included in the ERC-20 specification

While the decimals() function isn't originally included in the ERC-20 standard, it was introduced later as an optional add-on. Given this, not all valid ERC20 tokens implement this interface.

Therefore, indiscriminately casting all tokens to this interface and subsequently invoking this function can be risky

File: contracts/Stabilizer.sol

}39:         scalingFactor = 10 ** (ERC20(address(stableToken)).decimals() - IERC20Metadata(_collateralToken).decimals());

39

File: contracts/TokenToPriceFeed.sol

}96:         uint256 _decimals = erc20.decimals();

96

[L-03] safeApprove() is deprecated

The function has been marked as deprecated and is recommended to be replaced with safeIncreaseAllowance() and safeDecreaseAllowance().

If you're solely setting the allowance to a value signifying infinite, you can utilize safeIncreaseAllowance() as a substitute.

While the function might operate correctly now, any discovered flaws in this OpenZeppelin version, coupled with a mandatory upgrade to a version lacking this function, could result in unforeseen hold-ups in adapting and evaluating alternative contracts.

File: contracts/VaultFactoryZapper.sol

}80:             IERC20(_collateralToken).safeApprove(

80

File: contracts/LiquidationRouter.sol

}180:         IERC20(_collateral).safeApprove(stabilityPool, 0);
}181:         IERC20(_collateral).safeApprove(stabilityPool, _amount);
}183:         IERC20(_collateral).safeApprove(auctionManager, 0);
}184:         IERC20(_collateral).safeApprove(auctionManager, _amount);

180, 181, 183, 184

File: contracts/Vault.sol

}548:             IERC20(_collateral).safeApprove(
}552:             IERC20(_collateral).safeApprove(

548, 552

File: contracts/AuctionManager.sol

}298:             collateralToken.safeApprove(address(_lastResortLiquidation), 0);
}299:             collateralToken.safeApprove(

298, 299

[L-04] Use Ownable2Step.acceptOwnership() instead of Ownable.transferOwnership

Better use Ownable2Step.acceptOwnership() because it is more secure due to 2-stage ownership transfer.

File: contracts/MintableTokenOwner.sol

}4: import '@openzeppelin/contracts/access/Ownable.sol';
}31:         token.transferOwnership(_newOwner);

4, 31

File: contracts/TokenToPriceFeed.sol

}126:         Ownable.transferOwnership(_newOwner);

126

[L-05] Note that symbol() is not included in the ERC-20 standard

The symbol() function isn't part of the ERC-20 standard but was introduced as an optional extension.

Consequently, not all valid ERC20 tokens support this interface.

Blindly casting tokens to this interface and calling the function can be unsafe.

File: contracts/TokenToPriceFeed.sol

}110:             erc20.symbol(),

110

[L-06] ERC20 tokens with more than 18 decimals cause reverts

While the protocol is compatible with tokens containing 18 decimals or fewer, it may encounter issues with tokens having more than 18 decimals, such as YAMv2 with 24 decimals. Specifically, the following code snippet may encounter underflows:

File: contracts/TokenToPriceFeed.sol

}148:   uint256 _normalizedCollateralAmount = _auction.collateralAmount[i] *
                (10 ** (18 - _priceFeed.decimals(_auction.collateral[i])));

148

DanailYordanov commented 5 months ago

L3 ne e valid i na L1 vtoriq primer ne e valid, za purviq ne sum siguren

DanailYordanov commented 5 months ago

L4 sushto ne bi trqbvalo da e issue, shoto realno picha si vredi na sebe si, toi samo ima pravo da vikne taq funkciq i nqma smisul da q kara da revertva

DanailYordanov commented 5 months ago

mahnal sum l3,l1 i l4, dobavil sum tva poslednoto issue, ma me murzi da mu slagam ruchno vsichki occurences

0xMilenov commented 5 months ago

to nqma nujda ot vsichki edin dva primera e dostatuchno, samo che nqkude bi trqqlo da imashe otnosno decimals-a proverka 0 - 18, otnosno poslednoto , pak porazgledai kato mojesh

DanailYordanov commented 5 months ago

Значи е тука ще ревъртне - TokenToPriceFeed.sol#L96-L97 И тук също - Stabilizer.sol#L39. Но като цяло първото е самия guard, който не позволява админите да добавят с повече, ама като цяло идеята ми беше, че то нямат да имат възможността да добавят в бъдеще, ако искат, а не е сложно да решат тоя проблем с една функция например:

int8 decimalsDifference = 18 - int8(asset.token.dec);

if (decimalsDifference > 0) {
       _portion = _portion * (10 ** uint256(int256(decimalsDifference)));
   } else if (decimalsDifference < 0) {
       _portion = _portion / (10 ** uint256(int256(-decimalsDifference)));
   } 

Тоя код е от друг репорт, ма показва кво имам предвид. Ще оправя issue-то, да е по-ясно, но според теб дали да не го пишем NC

DanailYordanov commented 5 months ago

Също така имам още 2 issue-та да добавя тука дето намерих, ма после мисля, а не знам дали ти няма днес да му качваш low-овете на тоя пич