TierByConstruction Manual Review Findings

TBC-01M: Counter-Intuitive Tier Check

Type	Severity	Location
Logical Fault		TierByConstruction.sol:L112-L115

Description:

The tier check enforced by the onlyTier modifier is insecure and does not conform to the Checks-Effects-Interactions pattern as it performs a check post-execution.

Example:

Copy
95/// Modifier that restricts access to functions depending on the tier
96/// required by the function.
97///
98/// `isTier` involves an external call to tierContract.report.
99/// `require` happens AFTER the modified function to avoid rentrant
100/// `ITier` code.
101/// Also `report` from `ITier` is `view` so the compiler will error on
102/// attempted state modification.
103//  solhint-disable-next-line max-line-length
104/// https://consensys.github.io/smart-contract-best-practices/recommendations/#use-modifiers-only-for-checks
105///
106/// Do NOT use this to guard setting the tier on an `ITier` contract.
107/// The initial tier would be checked AFTER it has already been
108/// modified which is unsafe.
109///
110/// @param account_ Account to enforce tier of.
111/// @param minimumTier_ Minimum tier for the account.
112modifier onlyTier(address account_, uint256 minimumTier_) {
113    _;
114    require(isTier(account_, minimumTier_), "MINIMUM_TIER");
115}

Recommendation:

We advise the check to either be enforced both at the start and end of the execution or the function to be renamed as it implies a security guarantee that is enforced after execution and thus can lead to misuse. Examples are not limited to the advice of not setting the tier on the ITier contract as for example a selfdestruct instruction halts execution and would cause the contract to be delete-able by any tier. Additionally, the tier can be retrieved at the start of the modifier and utilized after the main function block (_) as modifier declarations permit local variables before and after the special underscore character.

Alleviation:

The Rain Protocol team stated that the TierByConstruction contract has been deprecated and will be removed soon. As a result, we consider this exhibit acknowledged.