Summary & Vulnerability Details
There is no check inside decreaseCollateral() to stop a user from calling it with param amount as zero.
File: contracts/facets/ShortRecordFacet.sol
82 @> function decreaseCollateral(address asset, uint8 id, uint88 amount)
83 external
84 isNotFrozen(asset)
85 nonReentrant
86 onlyValidShortRecord(asset, msg.sender, id)
87 {
88 STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];
89 short.updateErcDebt(asset);
90 if (amount > short.collateral) revert Errors.InsufficientCollateral();
91
92 short.collateral -= amount;
93
94 uint256 cRatio = short.getCollateralRatio(asset);
95 if (cRatio < LibAsset.initialMargin(asset)) {
96 revert Errors.CollateralLowerThanMin();
97 }
98
99 uint256 vault = s.asset[asset].vault;
100 s.vaultUser[vault][msg.sender].ethEscrowed += amount;
101
102 LibShortRecord.disburseCollateral(
103 asset, msg.sender, amount, short.zethYieldRate, short.updatedAt
104 );
105 emit Events.DecreaseCollateral(asset, msg.sender, id, amount);
106 }
This benefits the user in no way but causes loss of gas for him.
The following PoC shows that it is possible to call decreaseCollateral() with zero amount. Add the code inside test/Yield.t.sol and run through command forge test --mt testdecreaseCollateralByZeroIsAllowed -vv.
function testdecreaseCollateralByZeroIsAllowed() 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.decreaseCollateral(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
82 function decreaseCollateral(address asset, uint8 id, uint88 amount)
83 external
84 isNotFrozen(asset)
85 nonReentrant
86 onlyValidShortRecord(asset, msg.sender, id)
87 {
+ if (amount == 0) revert Errors.PriceOrAmountIs0();
88 STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];
89 short.updateErcDebt(asset);
90 if (amount > short.collateral) revert Errors.InsufficientCollateral();
91
92 short.collateral -= amount;
93
94 uint256 cRatio = short.getCollateralRatio(asset);
95 if (cRatio < LibAsset.initialMargin(asset)) {
96 revert Errors.CollateralLowerThanMin();
97 }
98
99 uint256 vault = s.asset[asset].vault;
100 s.vaultUser[vault][msg.sender].ethEscrowed += amount;
101
102 LibShortRecord.disburseCollateral(
103 asset, msg.sender, amount, short.zethYieldRate, short.updatedAt
104 );
105 emit Events.DecreaseCollateral(asset, msg.sender, id, amount);
106 }
