This audit looks into Pull Request 80 as seen here, including every change made in it. This includes the changes in the following contracts:
Note: This report does not provide any guarantee or warranty of security for the project.
1. [MED] TRSRY/OlympusTreasury.sol#L140 - functions
setDebt can’t be shutdown in emergency
- This commit does not provide full emergency protection,
repayDebthandles token transfers any time and should also be possible to pause when emergency is needed.
- In addition,
setDebtmakes state changes affecting
repayDebt, which also able to work at any time.
- Other state changing functions may or may not need to have the modifier to prevent any actions.
- There can be scenarios in which any repayment operation would be unwanted, such as a critical vulnerability of the Treasury or periphery contracts. In such a case, the borrowers shouldn’t be able to repay their debts, as the borrower side would lose their assets but the lender couldn’t receive the repayment.
- On the contrary, in other cases like non-security related operations shutdowns, shutting down loan repayment could result in unintended bad user experience for the borrower side, as a lending policy/off-chain lending logic may utilize these functions to track interest, and borrowers may require to pay more interest than what they intended as a side effect.
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.
2. [MED] Distributor.sol#L140-L143 A single pool with 0 (or dust) OHM balance will revert entire call
- On OlympusMinter.sol#L35
mintOhmfunction checks whenever 0 amount is passed and reverts if that’s the case.
- Because of this check, if
nextRewardForreturns 0 for any
pools, by having 0 OHM token balance, the entire call to
distributewill revert as
mintOhmcall of the respective iteration would revert.
Suggestion: To prevent this issue, the following fix can be applied:
- Some functions such as
setDebtdo not have
permissionedmodifier, allowing to use them directly.
- In addition, those functions are also used in following policies:
- This is the only module that a user can interact directly, without requiring a policy, necessarily violating design of module contracts.
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.
4. [INFO] module/MINTR: inaccurate param name for
decreaseMintApproval virtual declare
Suggestion: Rename first parameter
Emergency.sol has two errors and one event that are never used or called.
TreasuryCustodian.sol contains an unused custom error,
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_.