Potential User-Defined Value Type

[thedavidmeister]

TCE-01C: Potential User-Defined Value Type

Type	Severity	Location
Code Style	Informational	TierwiseCombine.sol:L39, L40

Description:

Given that the "report" type is a special purpose uint256 value, the legibility of the codebase and library usages can be optimized by defining a custom user-value type.

Example:

uint256 newerBlock_ = TierReport.tierBlock(newerReport_, tier_);
uint256 olderBlock_ = TierReport.tierBlock(olderReport_, tier_);
uint256 diff_ = newerBlock_.saturatingSub(olderBlock_);
ret_ = TierReport.updateBlockAtTier(ret_, tier_ - 1, diff_);

Recommendation:

We advise one to be defined for the "report" data type (i.e. type Report is uint256;) to optimize library usages and permit statements such as using TierReport for Report; without affecting the functions exposed by the "basic" data types (i.e. avoid using TierReport for uint256;). This finding will not be replicated across the codebase, however, it is applicable to many instances.

[thedavidmeister]

The compiler is complaining about this when it comes to do math with reports like:

TypeError: Explicit type conversion not allowed from "Report" to "uint256".
  --> contracts/tier/libraries/TierReport.sol:50:28:
   |
50 |                 if (uint32(uint256(report_ >> (i_ * 32))) > blockNumber_) {
   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

for

    function tierAtBlockFromReport(Report report_, uint256 blockNumber_)
        internal
        pure
        returns (uint256)
    {
        unchecked {
            for (uint256 i_ = 0; i_ < 8; i_++) {
                if (uint32(uint256(report_ >> (i_ * 32))) > blockNumber_) {
                    return i_;
                }
            }
            return TierConstants.MAX_TIER;
        }
    }

If I can't even explicitly cast from type Report is uint256; to uint256 that's a problem :/

[thedavidmeister]

TCE-01C: While we understand the issue, the rationale behind the user-defined type is to wrap operations like that under legible names. For example, uint256(report_ >> (i_ * 32)) could be wrapped to report_.shift32(i_). Additionally, whenever a custom defined type is defined two functions are always exposed labelled as wrap and unwrap, the first of which accepts an argument to cast to the special type and the latter of which returns the underlying type.

[thedavidmeister]

I'm concerned that if we go as far as to add wrapper functions then there will be additional gas cost, but maybe that is optimized away by the compiler.

I also think if we go down this road then this kind of handling of uint256 values as an implicit array of uint32 values, that would be treated in a generic way and then tiers would just be implemented on top of it.

Either way it is not urgent so I think we come back to it after the next audit.

[thedavidmeister]

Sounds good to us.