Platform: Code4rena
Start Date: 01/07/2021
Pot Size: $100,000 USDC
Total HM: 10
Participants: 7
Period: 7 days
Judge: ghoulsol
Total Solo HM: 4
Id: 17
League: ETH
Rank: 5/7
Findings: 4
Award: $8,124.11
🌟 Selected for report: 3
🚀 Solo Findings: 0
pauliax
The condition should be AND, not OR and err msg looks weird here: function distributeStrategyGainLoss(uint256 gain, uint256 loss) external override { uint256 index = vaultIndexes[msg.sender]; require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor"); Now basically every index is considered as valid. Bypasing this check allows an attacker to pass arbitrary values for 'gain' and 'loss' parameters to be distributed.
require(index > 0 && index <= N_COINS + 1, "<err_msg>");
#0 - flabble-gro
2021-07-14T20:24:24Z
Duplicate of #69
#1 - ghoul-sol
2021-07-26T03:48:00Z
Duplicate of #69
pauliax
An address can be whitelisted in safeAddresses but this cannot be undone later in case e.g. address becomes malicious.
Add a function to remove an address from safeAddresses.
#0 - flabble-gro
2021-07-14T20:24:45Z
Duplicate of #51
#1 - ghoul-sol
2021-07-26T02:36:15Z
Duplicate of #51 so medium risk.
🌟 Selected for report: pauliax
1307.9994 USDC - $1,308.00
pauliax
The check should be inclusive here to cover the case when totalAssets = withdrawalUsd: require(totalAssets > withdrawUsd, "totalAssets < withdrawalUsd");
require(totalAssets >= withdrawUsd, "totalAssets < withdrawalUsd");
#0 - kitty-the-kat
2021-07-14T16:05:23Z
Unsure what part is being referenced
#1 - kitty-the-kat
2021-07-15T11:53:21Z
Edge case that is unlikely to cause issues as gro protocol provides initial seed investment
🌟 Selected for report: pauliax
1307.9994 USDC - $1,308.00
pauliax
FixedStablecoins constructor does not validate that addresses in the array are not empty, != address(0), and relies that the creator passes the correct values for decimals. The comment next to USDC (0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48) says that it is supposed to have 6 decimals: uint256 public immutable USDC_DECIMALS; // = 1E6; However, when querying the actual value on Etherscan, it shows 0 decimals: https://etherscan.io/address/0xa2327a938febf5fec13bacfb16ae10ecbc4cbdcf#readContract The problem with USDC is that it uses a proxy pattern thus the implementation could change (decimals could change but in practice, I think it is very unlikely).
I think it would be better not to pass decimals separately and rely on the correctness of the input but use IERC20Detailed and query the decimals in code. Always querying the decimals on the go may be very inefficient and bring new attack vectors so I think you need to do here an assumption that decimals of upgradeable tokens won't change.
#0 - kitty-the-kat
2021-07-14T16:13:35Z
We dont expect to see any changes to underlying decimals of stablecoins, worst case scenario we can redeploy the affected contracts
🌟 Selected for report: pauliax
210.7126 USDC - $210.71
pauliax
function withdrawToAdapter re-assigns the amount but does not use it anywhere later. The declaration of this function could be changed to return this value to indicate the actual withdrawal amount. Also, I think this function should be included in IVault interface as it is externally called.
Make withdrawToAdapter return the amount that was withdrawn and also include this function in the IVault interface.
#0 - kitty-the-kat
2021-07-14T15:15:10Z
Not sure of reasoning here, never called by another contract - also, any transfer event of the underlying token would be seen.
#1 - ghoul-sol
2021-07-26T03:28:01Z
Reassigning the amount variable feels like unfinished business. Not sure why it's done, I'll keep this as gas optimization.