VIS-2 / taobank-04-24

0 stars 0 forks source link

Gas Optimizations #7

Closed 0xMilenov closed 4 months ago

0xMilenov commented 5 months ago
Issue Instances Gas Savings
[G-01] Missing validation check in redeemReward 2 6
[G-02] Store array length outside loops for efficiency 4 3
[G-03] Revert strings are less gas-efficient than custom errors 1 50
[G-04] Increments/decrements can be unchecked in for-loops 7 25

[G-01] Missing validation check in redeemReward

Context StabilityPool

Impact Users with a zero deposit are allowed to execute the function, potentially causing unnecessary gas expenditure.

Description

function redeemReward() external {
    Snapshots memory snapshots = depositSnapshots[msg.sender];
    uint256 contributorDeposit = deposits[msg.sender];

    uint256 compoundedDeposit = _getCompoundedDepositFromSnapshots(contributorDeposit, snapshots);
    _redeemReward();
    _updateDepositAndSnapshots(msg.sender, compoundedDeposit);
}

Recomendation. Add validation check.

function redeemReward() external {
    Snapshots memory snapshots = depositSnapshots[msg.sender];
    uint256 contributorDeposit = deposits[msg.sender];
+  require(contributorDeposit > 0, "deposit-is-0")

    uint256 compoundedDeposit = _getCompoundedDepositFromSnapshots(contributorDeposit, snapshots);
    _redeemReward();
    _updateDepositAndSnapshots(msg.sender, compoundedDeposit);
}

[G-02] Store array length outside loops for efficiency

When array length isn't cached, the Solidity compiler repeatedly reads it in every loop iteration.

For storage arrays, this means an added sload operation costing 100 extra gas for each subsequent iteration.

For memory arrays, it's an additional mload operation costing 3 extra gas post the first iteration.

File: contracts/LiquidationRouter.sol

}145:         for (uint256 i = 0; i <  collateralSet.length(); i++) {

145

File: contracts/Vault.sol

}199:         for (uint256 i = 0; i <  collateralSet.length(); i++) {
}496:         for (uint256 i = 0; i <  collateralSet.length(); i++) {
}537:         for (uint256 i = 0; i <  collateralSet.length(); i++) {

199, 496, 537

[G-03] Revert strings are less gas-efficient than custom errors

From Solidity v0.8.4 onward, custom errors have been introduced.

These errors provide a savings of approximately 50 gas each instance they're triggered, as they circumvent the need to allocate and keep the revert string.

Omitting these strings also conserves gas during deployment.

Furthermore, custom errors are versatile, applicable both inside and outside contracts, including in interfaces and libraries.

As stated in the Solidity blog:

With the introduction of Solidity v0.8.4, a streamlined and gas-efficient method has been provided to elucidate to users the reasons behind an operation's failure through custom errors.

Prior to this, although strings could be used to detail failure reasons (e.g., revert('Insufficient funds.');), they were notably costlier, especially in terms of deployment, and incorporating dynamic information into them was challenging.

It's advisable to transition all revert strings to custom errors in your solution, especially focusing on those that appear multiple times.

File: contracts/Stabilizer.sol

}34:         require(_mintableTokenOwner != address(0), "mintable-token-owner-is-zero");
}35:         require(_collateralToken != address(0), "collateral-token-is-zero");
}45:         require(_feeRecipient != address(0), "fee-recipient-is-zero");
}51:         require(_feeBps < = 500, "fee-too-high");
}57:         require(_amount  >= scalingFactor, "amount-too-small");
}72:         require(_amount  >= scalingFactor, "amount-too-small");

34, 35, 45, 51, 57, 72

[G-04] Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

The change would be:

- for (uint256 i; i <  numIterations; i++) {
+ for (uint256 i; i <  numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

These save around 25 gas saved per instance.

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existent for uint256.

File: contracts/VaultBorrowRate.sol

}28:         for (uint256 i; i <  _collateralsLength; i++) {

28

File: contracts/LiquidationRouter.sol

}145:         for (uint256 i = 0; i <  collateralSet.length(); i++) {
}242:         for (uint256 i; i <  _length; i++) {

145, 242