code-423n4 / 2021-05-88mph-findings

0 stars 0 forks source link

Gas optimizations - Structs over multiple mappings #22

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

a_delamo

Vulnerability details

Impact

PercentageFeeModel.sol is using multiple mappings for keeping track of the fees.

  struct FeeOverride {
    bool isOverridden;
    uint256 fee;
  }

  mapping(address => FeeOverride) public interestFeeOverrideForPool;
  mapping(address => FeeOverride) public earlyWithdrawFeeOverrideForPool;
  mapping(address => mapping(uint256 => FeeOverride)) public interestFeeOverrideForDeposit;
  mapping(address => mapping(uint256 => FeeOverride)) public earlyWithdrawFeeOverrideForDeposit;

I would recommend using a single mapping with a struct and inside of multiple mappings as is less gas efficient. Here more info: https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

Especially, given that some functions are currently accessing multiple mappings.

Also, if fees can not be zero. We could remove the FeeOverride struct to just use uint, but I guess is not possible.

ZeframLou commented 3 years ago

I don't think this actually saves gas, as it won't decrease the amount of storage used, which is how the linked Medium article saves gas.

ghoul-sol commented 3 years ago

Report is too broad to confirm gas savings. Mentioned article touches two aspects, data encoding and tight data packing by structs. Implementation that uses single struct, data encoding and potentially saves gas is not obvious in this case, and neither are its gas savings as it's not clear how the contract is going to be used in practice. One could see a scenario where (more expensive) separate setters makes more sense because there's no need to update all values every time.

Closing as invalid.