PermissionBuilder Manual Review Findings

PBR-01M: Removal of Important Security Check

Type	Severity	Location
Input Sanitization		PermissionBuilder.sol:L160, L161, L166

Description:

The PermissionBuilder::setAllowance function contained an if-revert check that ensured the balance being established for the key is less-than-or-equal-to the maxBalance dynamically calculated within the function.

This condition was removed and affects out-of-scope contract AllowanceTracker and specifically AllowanceTracker::_accruedAllowance.

The function will improperly reset the balance of the key to the maxBalance whenever an interval elapses even if the balance is significantly higher than the maxBalance, effectively reducing it.

Impact:

An invoker of the PermissionBuilder::setAllowance function would expect the balance allowed to solely decrease when consumed rather than when updated which can cause significant misbehaviours into how users as well as protocols interact with the PermissionBuilder.

Example:

Copy
158function setAllowance(
159    bytes32 key,
160    uint128 balance,
161    uint128 maxBalance,
162    uint128 refill,
163    uint64 period,
164    uint64 timestamp
165) external onlyOwner {
166    maxBalance = maxBalance > 0 ? maxBalance : type(uint128).max;
167
168    allowances[key] = Allowance({
169        refill: refill,
170        period: period,
171        timestamp: timestamp,
172        balance: balance,
173        maxBalance: maxBalance
174    });

Recommendation:

We advise the code to re-introduce the if-revert pattern check present in the original code to ensure that the AllowanceTracker behaves as expected.

Alleviation (e6d315f9170dcf4c622d504bd2fb6eafbdac9b75):

The AllowanceTracker and PermissionBuilder contracts were slightly refactored to utilize a maxRefill notion rather than maxBalance notion, with the AllowanceTracker::_accruedAllowance function measuring whether the present balance is lower than the maxRefill value and increasing it by the elapsed periods.

Should the balance after the refill is executed exceed the maxRefill value, it will be set to it to avoid the balance exceeding the maxRefill value when it is refilled.

As the contract no longer improperly resets the balance to the refill amount if it has been set to a greater value, we consider this exhibit fully alleviated.

We would like to note that a side-effect of the current implementation is that refill periods that have elapsed in the past whilst the balance is greater than the refill threshold will be unaccounted for when the balance needs to refill again.

We consider this a desirable trait that the Gnosis Guild team wishes to enforce.