We audited the OriginProtocol/origin-dollar repository at commit 4495130.
In scope was the following file:
contracts
└── contracts
└── token
└── OUSD.sol
This audit focuses on reviewing an upgrade to the existing OUSD contract, which implements a rebasing ERC-20 token. The primary objective of the upgrade is to enable yield delegation, allowing an account to seamlessly transfer all earned yield to another account. Additionally, the upgrade addresses minor rounding issues in the contract's internal accounting mechanism.
Previously, there were only two account types in the system: rebasing and non-rebasing.
The current upgrade introduces two new account types to enhance functionality:
It is important to note that yield delegation operates as a 1:1 relationship. For instance, if Account A delegates yield to Account B, then A cannot delegate yield to any other account, and B cannot receive yield from any other source. Additionally, an account cannot simultaneously function as both a yield delegator and a receiver.
An initial overview of the security considerations and trust assumptions is available in the previous Origin Dollar Audit report.
The upgrade reviewed in this audit introduces additional points for consideration:
rebasingCreditsPerToken remains significantly higher than 1e18, as per the current implementation. If this value were to decrease substantially, malicious users could exploit the system by inflating their balance through operations that repeatedly trigger rounding in their favor. For context, the current rebasingCreditsPerToken value is approximately 6.76e26.The system defines two roles with privileged access:
This audit assumes that the entities managing these roles always act as intended. Consequently, potential attacks or vulnerabilities involving misuse of these privileged roles were not considered within the scope of this audit.
In the current implementation, the Governor role has full control over yield delegations, leading to several potential issues:
To address these concerns, consider granting users greater autonomy over their yield. For instance, the system could allow users to specify which accounts they wish to delegate to or receive yield from, as well as provide an option to opt in or out of yield delegation altogether.
Update: Acknowledged, not resolved. The Origin Protocol team stated:
Yield delegation being solely under the control of token holder governance is by design.
Within OUSD.sol, multiple instances of missing docstrings were identified:
TotalSupplyUpdatedHighres eventAccountRebasingEnabled eventAccountRebasingDisabled eventTransfer eventApproval eventYieldDelegated eventYieldUndelegated eventtotalSupply state variablevaultAddress state variablerebaseState state variableyieldTo state variableyieldFrom state variableinitialize functionsymbol functionname functiondecimals functionConsider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #2319.
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled. However, the OUSD.sol file has the solidity ^0.8.0 floating pragma directive.
To improve reliability and avoid unintended issues caused by differences between compiler versions, consider using a fixed pragma directive.
Update: Acknowledged, not resolved. The Origin Protocol team stated:
For now, we will keep using the Solidity version in our build configuration since that version works better with our current tooling.
Multiple instances of redundant code were identified in the OUSD contract:
initialize function cannot be executed since the contract was already initialized in previous versions. To avoid unnecessarily increasing code size, consider removing the function or commenting it out for future reference.delegateYield function, checking both the yieldFrom/yieldTo mappings and the rebaseState mapping is redundant, as the first check will only pass if the second passes, and vice versa. Consider removing the first check, as it involves more storage reads than the second one.undelegateYield function, there is no need to set the credit balance of the delegation source, as it was already set to their balance within delegateYield.Consider removing these redundancies to enhance the clarity and efficiency of the codebase.
Update: Acknowledged, not resolved. The Origin Protocol team stated:
We require the
initializefunction for future token contract deployments that inheritOUSD.sol. Moreover, we intentionally set the credit balance inundelegateYieldand check both mappings indelegateYieldto prevent future changes from introducing an error in this critical part. Also, both of these functions are rarely called.
Within OUSD.sol, the return value of the transferFrom and approve functions is not documented.
Consider thoroughly documenting all functions and events that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #2321.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Within OUSD.sol, multiple instances of mappings without named parameters were identified:
allowances state variablecreditBalances state variablealternativeCreditsPerToken state variablerebaseState state variableyieldTo state variableyieldFrom state variableConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Acknowledged, not resolved. The Origin Protocol team stated:
We are using an older version of Solidity and cannot use named parameters in mappings at this time.
Multiple optimizable storage reads and writes were identified in the OUSD contract:
_creditsPerToken function, the alternativeCreditsPerToken mapping is accessed twice for the same key.changeSupply function, the totalSupply, rebasingCredits_, and rebasingCreditsPerToken_ state variables are read multiple times.undelegateYield function, the yieldTo mapping is accessed twice for the same key._adjustAccount function, the alternativeCreditsPerToken for non-rebasing accounts is always set to 1e18, even when the mapping already has that value.To lower gas consumption, consider reducing unnecessary storage reads by caching these values in memory variables, and only write to storage when the value needs to be updated.
Update: Resolved in pull request #2322 and pull request #2325. The Origin Protocol team stated:
_creditsPerTokenhas been optimized as suggested.changeSupplyis rarely called and, in this instance, we prefer code readability over gas optimization.undelegateYieldis rarely called and, in this instance, we prefer code readability over gas optimization._adjustAccounthas been optimized as suggested.
Throughout the OUSD contract, there are multiple instances of incorrect documentation and typographical errors:
Consider correcting these instances to improve the quality of the documentation.
Update: Resolved in pull request #2323.
This audit focused on an upgrade to the OUSD contract, with the primary change being the ability to delegate yield from one account to another. We did not identify any major issues with the upgrade; it remains compatible with the previous version, and the contract should continue to function as intended following the implementation update. While some cases result in the protocol rounding balances in favor of users, the resulting gains are negligible and unlikely to pose any issues or be exploitable by malicious actors.
We provided several recommendations to enhance the overall quality of the codebase. The Origin team was cooperative and responsive to our questions throughout the audit process.