code-423n4 / 2021-07-pooltogether-findings

0 stars 0 forks source link

[Optimization] Shorten revert strings larger than 32 bytes #68

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

hrkrshnn

Vulnerability details

Consider shortening revert strings

Several revert strings in SwappableYieldSource.sol are more than 32 bytes. Try to keep everything below 32 bytes. Even better would be replacing it with Solidity's custom errors from 0.8.4.

Example:

  function transferERC20(IERC20Upgradeable erc20Token, address to, uint256 amount) external onlyOwnerOrAssetManager returns (bool) {
    require(address(erc20Token) != address(yieldSource), "SwappableYieldSource/yield-source-token-transfer-not-allowed");
    ...
}

The above string is 60 characters. Everything above 32 characters would need an additional mstore. This would incur a cost for an additional mstore, cost for memory expansion, as well as additional stack operation costs. This is only relevant when the revert condition is met.

Shortening would also reduce the deploy cost for the contract in all cases.

If possible, it's recommended to change this to:

pragma solidity ^0.8.4;

/// A long NatSpec comment explaning this in detail, without costing on chain gas.
error YieldSourceTokenTransferNotAllowed();

function transferERC20(IERC20Upgradeable erc20Token, address to, uint256 amount) external onlyOwnerOrAssetManager returns (bool) {
    if (address(erc20Token) == address(yieldSource))
        revert YieldSourceTokenTransferNotAllowed();
    ...
 }

The above change will also decrease gas.

There are several other instances where large revert strings are used. All of them can be shortened / replaced.

PierrickGT commented 3 years ago

This feature is only available for Solidity version 0.8.4, we are using version 0.7.6 in the swappable yield source and 0.8.2 in the mStable yield source.

0xean commented 3 years ago

duplicate of #27

PierrickGT commented 3 years ago

Closing as this issue is a duplicate.