search

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

0 stars 0 forks source link

Gas optimizations - storage over memory #19

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

a_delamo

Vulnerability details

Impact

Some functions are using memory to read state variables when using storage is more gas efficient.

function withdrawableAmountOfDeposit(
        uint256 depositID,
        uint256 virtualTokenAmount
    ) external view returns (uint256 withdrawableAmount, uint256 feeAmount) {
        // Verify input
        //FIXME: Storage
        Deposit memory depositEntry = _getDeposit(depositID);
        if (virtualTokenAmount == 0) {
            return (0, 0);
        } else {
            if (virtualTokenAmount > depositEntry.virtualTokenTotalSupply) {
                virtualTokenAmount = depositEntry.virtualTokenTotalSupply;
            }
        }
    ...
}

  function _getVestWithdrawableAmount(uint256 vestID)
    internal
    view
    returns (uint256 withdrawableAmount)
  {
    // read vest data
    //FIXME: Storage
    Vest memory vestEntry = _getVest(vestID);
    DInterest pool = DInterest(vestEntry.pool);
    DInterest.Deposit memory depositEntry =
      pool.getDeposit(vestEntry.depositID);

   ...
   }

The following functions contains memory than could be changed to storage:

https://docs.soliditylang.org/en/v0.4.21/types.html#reference-types

Recommended Mitigation Steps

ZeframLou commented 3 years ago

The memory location of those variables are intentionally set to be memory to only load the data from storage once, in order to save gas. If changed to storage there will be multiple SLOADs for accessing the same variable, which is expensive.

ghoul-sol commented 3 years ago

In case of withdrawableAmountOfDeposit function, warden is right. _getDeposit makes 7 SLOAD calls in this case because it's pulling whole object from storage. Using storage would make only 5 SLOAD calls.

In case of _getVestWithdrawableAmount, either way uses 6 SLOAD calls.

Warden is right in at least one case so the reports stands.

ZeframLou commented 3 years ago

Addressed in https://github.com/88mphapp/88mph-contracts/commit/6459177a642d854ca6ee4deeda7f61075bd4bdf1