Summary & Vulnerability Details
There is no check inside increaseCollateral() to stop a user from calling it with param amount as zero.
If the cRatio is already greater than or equal to LibAsset.primaryLiquidationCR(asset), this also unnecessarily calls short.resetFlag() here which resets the updatedAt variable if yield is more than zero.
File: contracts/facets/ShortRecordFacet.sol
38 @> function increaseCollateral(address asset, uint8 id, uint88 amount)
39 external
40 isNotFrozen(asset)
41 nonReentrant
42 onlyValidShortRecord(asset, msg.sender, id)
43 {
44 STypes.Asset storage Asset = s.asset[asset];
45 uint256 vault = Asset.vault;
46 STypes.Vault storage Vault = s.vault[vault];
47 STypes.VaultUser storage VaultUser = s.vaultUser[vault][msg.sender];
48 if (VaultUser.ethEscrowed < amount) revert Errors.InsufficientETHEscrowed();
49
50 STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];
51 short.updateErcDebt(asset);
52 uint256 yield = short.collateral.mul(short.zethYieldRate);
53 short.collateral += amount;
54
55 uint256 cRatio = short.getCollateralRatio(asset);
56 if (cRatio >= Constants.CRATIO_MAX) revert Errors.CollateralHigherThanMax();
57
58
59 if (cRatio >= LibAsset.primaryLiquidationCR(asset)) {
60 @> short.resetFlag();
61 }
62
63 yield += amount.mul(Vault.zethYieldRate);
64 short.zethYieldRate = yield.divU80(short.collateral);
65
66 VaultUser.ethEscrowed -= amount;
67 Vault.zethCollateral += amount;
68 Asset.zethCollateral += amount;
69 emit Events.IncreaseCollateral(asset, msg.sender, id, amount);
70 }
This benefits the user in no way but causes loss of gas for him.
The following PoC shows that it is possible to call increaseCollateral() with zero amount. Add the code inside test/Yield.t.sol and run through command forge test --mt testincreaseCollateralByZeroIsAllowed -vv.
function testincreaseCollateralByZeroIsAllowed() public {
fundLimitShort(DEFAULT_PRICE, 100000 ether, receiver);
fundLimitBid(DEFAULT_PRICE, 100000 ether, sender);
uint256 ethEscrowedInitial =
diamond.getVaultUserStruct(vault, receiver).ethEscrowed;
skip(yieldEligibleTime);
generateYield();
vm.prank(receiver);
diamond.increaseCollateral(asset, Constants.SHORT_STARTING_ID, 0);
uint256 ethEscrowedFinal = diamond.getVaultUserStruct(vault, receiver).ethEscrowed;
assertEq(
ethEscrowedInitial, ethEscrowedFinal, "Initial & final ethEscrowed not equal"
);
}
Impact
No value added for the user but results in loss of gas.
Tools Used
Manual inspection and foundry.
Recommendations
Add a constraint that amount should not be zero. A custom error Errors.PriceOrAmountIs0 already exists inside the protocol for that.
File: contracts/facets/ShortRecordFacet.sol
38 @> function increaseCollateral(address asset, uint8 id, uint88 amount)
39 external
40 isNotFrozen(asset)
41 nonReentrant
42 onlyValidShortRecord(asset, msg.sender, id)
43 {
+ if (amount == 0) revert Errors.PriceOrAmountIs0();
44 STypes.Asset storage Asset = s.asset[asset];
45 uint256 vault = Asset.vault;
46 STypes.Vault storage Vault = s.vault[vault];
47 STypes.VaultUser storage VaultUser = s.vaultUser[vault][msg.sender];
48 if (VaultUser.ethEscrowed < amount) revert Errors.InsufficientETHEscrowed();
49
50 STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];
51 short.updateErcDebt(asset);
52 uint256 yield = short.collateral.mul(short.zethYieldRate);
53 short.collateral += amount;
54
55 uint256 cRatio = short.getCollateralRatio(asset);
56 if (cRatio >= Constants.CRATIO_MAX) revert Errors.CollateralHigherThanMax();
57
58 //@dev reset flag info if new cratio is above primaryLiquidationCR
59 if (cRatio >= LibAsset.primaryLiquidationCR(asset)) {
60 short.resetFlag();
61 }
62
63 yield += amount.mul(Vault.zethYieldRate);
64 short.zethYieldRate = yield.divU80(short.collateral);
65
66 VaultUser.ethEscrowed -= amount;
67 Vault.zethCollateral += amount;
68 Asset.zethCollateral += amount;
69 emit Events.IncreaseCollateral(asset, msg.sender, id, amount);
70 }
