code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

Gas Optimizations #195

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Table of Contents

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./Creator/ZcToken.sol:31:    constructor(uint8 _protocol, address _underlying, uint256 _maturity, address _cToken, address _redeemer, string memory _name, string memory _symbol, uint8 _decimals) 
./Creator/Creator.sol:36:    string memory n,
./Creator/Creator.sol:37:    string memory s,
./Swivel/Swivel.sol:495:  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {
./Marketplace/MarketPlace.sol:68:    string memory n,
./Marketplace/MarketPlace.sol:69:    string memory s

Mitigation

Change memory to calldata

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, it acctually cost more gas to use elements smaller than 32 bytes. Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

./Creator/ZcToken.sol:17:    uint8 public immutable protocol;
./Creator/VaultTracker.sol:26:  uint8 public immutable protocol;
./Creator/Creator.sol:31:    uint8 p,
./Creator/Creator.sol:38:    uint8 d
./Swivel/Swivel.sol:35:  uint16 constant public MIN_FEENOMINATOR = 33;
./Swivel/Swivel.sol:37:  uint16[4] public feenominators;
./Swivel/Swivel.sol:495:  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {
./Marketplace/MarketPlace.sol:22:  mapping (uint8 => bool) public paused;
./Marketplace/MarketPlace.sol:65:    uint8 p,
./Marketplace/MarketPlace.sol:336:  function pause(uint8 p, bool b) external authorized(admin) returns (bool) {
./Marketplace/MarketPlace.sol:346:  modifier unpaused(uint8 p) {
./Marketplace/Compounding.sol:50:  function underlying(uint8 p, address c) internal view returns (address) {
./VaultTracker/VaultTracker.sol:26:  uint8 public immutable protocol;

Mitigation

I suggest using uint256 instead of anything smaller.

robrobbins commented 2 years ago

constructor args cannot be calldata, thus that suggestion is incorrect

robrobbins commented 2 years ago

fixes for the arrays in swivel.setFee (now changeFee) were addressed in a different chore.

calldata has been used where possible otherwise

robrobbins commented 2 years ago

https://github.com/Swivel-Finance/gost/pull/428