This audit looks into Pull Request 80 as seen here, including every change made in it. This includes the changes in the following contracts:MINTR.v1.sol
, OlympusMinter.sol
, OlympusTreasury.sol
, TRSRY.v1.sol
, BondCallback.sol
, Distributor.sol
, Emergency.sol
, Operator.sol
, TreasuryCustodian.sol
.
This audit was conducted by kebabsec members sai, FlameHorizon and okkothejawa.
Note: This report does not provide any guarantee or warranty of security for the project.
repayDebt
, setDebt
can’t be shutdown in emergencypermissioned
for some functionsincreaseMinterApproval
decreaseMintApproval
virtual declareShutdown
, custom error PolicyStillActive
and PolicyNotFound
.PolicyNotFound
.IOperator.sol
.repayDebt
, setDebt
can’t be shutdown in emergencyrepayDebt
handles token transfers any time and should also be possible to pause when emergency is needed.setDebt
makes state changes affecting repayDebt
, which also able to work at any time.Suggestion: To resolve the described trade-off, consider implementing two different shutdown mechanisms of different severity to cover the both cases, like a hard and soft shutdown.
mintOhm
function checks whenever 0 amount is passed and reverts if that's the case.nextRewardFor
returns 0 for any pool
in pools
, by having 0 OHM token balance, the entire call to distribute
will revert as mintOhm
call of the respective iteration would revert.Suggestion: To prevent this issue, the following fix can be applied:
for (uint256 i; i < poolLength; ) {
address pool = pools[i];
uint256 reward = nextRewardFor(pool);
if (pool != address(0) && reward > 0) {
MINTR.mintOhm(pool, reward);
IUniswapV2Pair(pool).sync();
}
unchecked {
i++;
}
}
permissioned
for some functionswithdrawReserves
, repayDebt
, setDebt
do not have permissioned
modifier, allowing to use them directly.TRSRY.withdrawReserves(msg.sender, payoutToken, outputAmount_);
.requests[1] = Permissions(TRSRY_KEYCODE, TRSRY.withdrawReserves.selector);
.TRSRY.withdrawReserves(msg.sender, reserve, amountOut);
.requests[4] = Permissions(TRSRY_KEYCODE, TRSRY.setDebt.selector);
Suggestion: Consider having consistent design of permissioned modules, and implement extra policies to act as front-ends for possible edge cases to fully adhere to the design principles. This issue can be taken as a more long-term suggestion rather than short-term priority.
increaseMinterApproval
decreaseMintApproval
virtual declareSuggestion: Rename first parameter minter_
to policy_
.
Shutdown
, custom error PolicyStillActive
and PolicyNotFound
Emergency.sol has two errors and one event that are never used or called.
PolicyNotFound
.TreasuryCustodian.sol
contains an unused custom error, PolicyNotFound
.
IOperator.sol
.The errors in Operator.sol
could be moved to it's corresponding interface contract.
In these lines, there's a ternary operation that reduces mint approval for a policy. In the case of the amount_
being equal to approval
, it uses the exppression approval - amount_
, which equals to 0, we think it would be more intuitive in the code if all the outcomes that should output 0, to be on the other expression of the ternary operation. We've also discovered during our testing that doing this reduces gas by around ~300, in the case described above.
Suggestion: Change the approval < amount_
to approval <= amount_
.