This security assessment was prepared by OpenZeppelin, protecting the open economy.
Table of Contents
- Table of Contents
- Summary
- Scope
- System Overview
- Privileged Roles
- Findings
- Critical Severity
- Medium Severity
- Low Severity
- Notes & Additional Information
- Constant set by constructor not marked immutable
- Contract file and contract name do not match
- Use of draft EIP code
- Imports not grouped together
- Misleading documentation
- Mismatched parameter name between interface and contract
- Typographical errors
- Unnecessary use of uint16
- Unnecessary inheritance
- Unused imports
- No functionality to withdraw portion of funds
- Conclusions
Summary
- Type
- DeFi
- Timeline
- From 2022-05-04
- To 2022-05-11
- Total Issues
- 19 (19 resolved)
Scope
We audited commit a95f0f6499be396c0a245f0183009b98cc72f123
of the PaymentsERC20Public
repository.
In scope were the following contracts:
contracts
├── ERC20
│ ├── ERC20Token.sol
├── Payments
│ ├── EIP712Verifier.sol
│ ├── FeesCollectors.sol
│ ├── IEIP712Verifier.sol
│ ├── IPaymentsERC20.sol
│ ├── Operators.sol
│ ├── PaymentsERC20.sol
├── Migrations.sol
System Overview
The Freeverse platform is a layer 2 for minting, evolving, and trading mutable NFTs with on-chain certifiable attributes (‘Living Assets’), as well as traditional immutable NFTs.
The audited system deals with the ERC20 token payments used for the trade of the assets. The PaymentsERC20
contract defines the type of ERC20
tokens used in the trade along with a paymentWindow
in which the payment against an asset needs to be completed.
The buyer
has to transfer an amount of ERC20
tokens to the PaymentsERC20
contract for the purchase of an asset. The terms of the purchase are agreed upon by the buyer
and seller
outside of this audited system in presence of a special user called the Operator
. The system assumes that the Operator
is a trusted third-party that would ensure that both the buyer
and the seller
would agree to the terms of the asset purchase.
The transfer of the ERC20
tokens from the buyer’s account can be triggered by either the Operator
by providing the buyer’s signature on the payment details, or by the buyer
by providing the Operator's
signature on the payment details. If a buyer
has a balance of ERC20
tokens deposited in this contract at the start of a new payment, the contract reuses the existing tokens, and only transfers from the buyer's
account if additional tokens are required.
Once the ERC20
tokens are transferred to this contract, they remain locked until the Operator
confirms the asset has been transferred from the seller
to the buyer
. The asset transfer itself is outside the scope of this audit.
If the asset transfer was successful, the amount of ERC20
tokens required for the asset purchase minus the fee is credited to the seller's
balance. If the asset transfer failed, then a refund is initiated to the local balance of the buyer
.
Anyone can trigger a refund
request on an asset purchase if the payment process is incomplete and the paymentWindow
of the purchase has expired. The amount of the payment is refunded to the buyer's
local balance.
The PaymentsERC20
contract does not transfer funds to the user’s account automatically. A user can call the withdraw
function to withdraw all of their tokens from the system.
Privileged Roles
Owner
The account that deploys this system has the privileged role of the owner
. The ownership can be renounced or transferred.
The owner
is responsible for:
- Setting the default fee collector
- Setting a universe fee collector
- Removing a universe fee collector
- Setting the default operator
- Setting a universe operator
- Removing a universe operator
- Setting a payment window
- Setting if seller registration is required
Operator
An operator
is an account which is responsible for ensuring that the buyer and seller are meeting the agreed terms of an asset purchase.
The operator provides their signature on the PaymentInput
which is required to transfer tokens from the buyer’s account to the system. The operator can trigger the transfer themselves with the buyer’s permission.
The operator also provides their signature on the AssetTransferResult
which indicates whether the asset transfer from the seller to the buyer was successful or not.
If the operator does not provide the asset transfer confirmation within the paymentWindow
, a refund can be initiated.
The role of the operator can be either default
which acts an operator of the entire system, or specific to a universe. At the time of deployment, the owner
is assigned as the default operator.
Update: In the current design of the payment system, the operator role wields a significant amount of power. A buyer cannot initiate a payment without the approval of an operator, and a seller must rely on an operator to vouch for the transfer of their asset. A corrupt operator has the ability to steal funds or assets from the system; the detailed attack is described in C01. However, after discussing with the Freeverse team, we have concluded that operators are explicitly trusted to be good actors in the system.
FeesCollector
Fee collector is the address where a fee is credited when a payment is successfully completed. The fee is paid in ERC20
tokens defined in the PaymentsERC20
contract. The system does not transfer the fee to the fee collector’s address automatically, but adds it to their internal balance.
The role of the fee collector can be either default
which acts a fee collector of the entire system, or specific to a universe. The system allows only one default fee collector. If a universe has a fee collector defined, the fee goes to the universe fee collector and not the default fee collector. At the time of deployment, the owner
is assigned as the default fee collector.
Findings
Here we present our findings.
Critical Severity
Operators are implicitly trusted
In the current design of the payment system, the operator role wields a significant amount of power. A buyer cannot initiate a payment without the approval of an operator, and a seller must rely on an operator to vouch for the transfer of their asset. Operators are implicitly trusted to be good actors in the system. However, there are four possible scenarios where operators could behave maliciously:
- Operator as a buyer: Both the buyer and operator need to agree upon the
PaymentInput
data. But there’s nothing restricting an operator from also acting as a buyer, in which case they can approve their own transactions. More importantly, acting as a buyer an operator could defraud a seller by obtaining a refund even though the asset was received. After the seller’s asset is transferred, the operator could either create a signedAssetTransferResult
with thewasSuccessful
field set tofalse
, or not sign anAssetTransferResult
at all. The operator could then callrefund
orrefundAndWithdraw
after the payment window has expired, and obtain their full payment amount even though the seller’s item was successfully transferred. - Operator as a seller: The contracts do not restrict an operator from also being a seller in a transaction they are responsible for monitoring. In this case, an operator could register as a seller and post an item for sale. After a buyer initiates payment, the operator could craft an
AssetTransferResult
with thewasSuccessful
field set totrue
, but then choose not to transfer the asset. The operator could then callfinalize
orfinalizeAndWithdraw
with the signedAssetTransferResult
, prior to the payment window expiring, thus receiving payment for an asset that was never delivered. - Operator colluding with a buyer: Similar to the case where the operator and the buyer are the same, an operator could work together with a buyer in an attempt to obtain a refund for a transferred asset. As before, the operator could create a signed
AssetTransferResult
withwasSuccessful
set tofalse
, even though the asset was transferred. - Operator colluding with a seller: Similar to the case where the operator and the seller are the same, an operator could work in conjunction with a seller who owns an item of value. As before, the operator would create a signed
AssetTransferResult
withwasSuccessful
set totrue
, even though the seller did not transfer the asset.
An operator should always be an observer of transactions and never participate as a buyer or seller. To prevent misuse of the power granted to an operator, consider adding checks which ensure that the buyer
and seller
addresses in the PaymentInput
struct can never be the same as the operator
address for the same payment transaction. Also consider revisiting the trust assumptions that allow an operator entity to independently vouch for the success or failure of an asset transfer via the AssetTransferResult
mechanism, and how the system can guarantee that an asset transfer result is always produced.
Update: Not an issue. After discussing with the Freeverse team, we have concluded that the system is designed with a trust assumption that the role of operator will always be in the favor of the system. Additionally, the team has applied the fixes in the codebase to ensure that an operator cannot be a buyer or a seller, this is fixed as of commit 714e99e
of PR #2.
Medium Severity
Potential loss of access to funds if payment window is incorrectly set
The _paymentWindow
variable within the PaymentsERC20
contract stores the maximum amount of time allowed for an asset transfer to be completed once a payment starts. The default value is 30 days and during this time, the buyer’s funds remain locked. If the buyer tries to obtain a refund before the payment window has expired, the refund request will not be accepted.
The PaymentsERC20
contract has a setPaymentWindow
function that allows the contract owner to adjust the payment window for future payments. This function does not impose a limit on the chosen window size, making it possible to set the value so high that obtaining a refund from an asset transfer that failed to complete would effectively be impossible.
To prevent potential loss of access to funds, within the setPaymentWindow
function, consider adding an upper limit on the payment window duration that the function will accept.
Update: Fixed as of commit a476fa1
of PR #3.
Low Severity
Duplicate code
Duplication of code is error prone because the repeated implementations can get out of sync as the codebase evolves, potentially leading to unexpected behavior. There two instances of code duplication in the PaymentsERC20
contract:
- The majority of the code in the
pay
andrelayedPay
functions inPaymentsERC20
is duplicated. Once the initial checks are completed in each case, all of the remaining steps are identical. Lines 102-123 inrelayedPay
are nearly identical to lines 141-162 inpay
, with the only difference being line 106 vs line 145, which are functionally equivalent in setting the operator. Consider creating a shared private function for this duplicate code. - The
maxFundsAvailable
function makes calls to the external ERC20 contract in order to obtain the buyer’s approved spend allowance for thePaymentERC20
contract and the buyer’s token balance in their wallet. To obtain allowance and balance information from the external contract,PaymentERC20
implementsallowance
anderc20BalanceOf
view functions that make respective calls to theallowance
andbalanceOf
functions of theIERC20
interface. However, whenmaxFundsAvailable
performs the allowance and balance queries, it calls the providedallowance
function, but not the providedbalanceOf
function. In the latter case,IERC20.balanceOf()
is called directly, duplicating the code in theerc20BalanceOf
function. Consider using theerc20BalanceOf
function to obtain the external token balanceerc20Balance
.
Update: Fixed as of commit 645f3f4
of PR #1.
Inconsistent and lack of indexed event parameters
In the IPaymentsERC20
contract, the Paid
event has an unindexed paymentId
parameter, even though paymentId
is indexed in the BuyerRefunded
and Payin
events.
In FeesCollectors.sol
, the DefaultFeesCollector
and UniversalFeesCollector
events do not apply the indexed
keyword to the feesCollector
or universeId
parameters.
In Operators.sol
, the DefaultOperator
and UniverseOperator
events do not apply the indexed
keyword to the operator
or universeId
parameters.
Indexed parameters are useful for quick offchain indexing of logs. Consider indexing applicable event parameters to support the searching and filtering abilities of offchain services.
Update: Fixed as of commit 745c035
of PR #4.
Missing docstrings
The following functions in the codebase lack documentation:
- All the functions, events, and state variables in the
FeesCollectors
contract are missing docstrings - All the functions, events, and state variables in the
Operators
contract are missing docstrings - The
PaymentInput
andAssetTransferResult
structs in theIEIP712Verifier
interface have no comments explaining any of the fields - None of the events in the
IPaymentsERC20
interface have any documentation - There are no docstrings in the
MyToken
contract to explain the contract’s purpose
This lack of documentation hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ 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: Fixed as of commit d311775
of PR #5. However, there are a few typographical errors in the newly added docstrings: – In the docstring above the MyToken
contract, “implementation” is misspelled as “implentation” – In the docstring above the AssetTransferResult
struct, “ASSET_TRANSFERRING” is misspelled as “ASSET_TRANSFERING”; PR #26 moves this line to ISignableStructs.sol
No check that seller is already registered
The registerAsSeller
function in the PaymentsERC20
contract emits a NewSeller
event when a user calls it. There is no check in registerAsSeller
to determine if a user address is already registered, so this function will emit a NewSeller
event every time it is called. Depending on how the event log is used, this may lead to incorrect behavior by external observers of the contract if a user calls the function more than once.
Consider checking the existing bool value of _isRegisteredSeller
when registerAsSeller
is called, and reverting if the user is already registered.
Update: Fixed as of commit 6c7a698
of PR #6.
Naming issues hinder code understanding and readability
To favor explicitness and readability, several parts of the contracts may benefit from better naming. Some suggestions are:
States
enum in theIPaymentsERC20
interface should beState
enum- In the
EIP712Verifier
, the variable nameinp
is confusing as it indicates bothPaymentInput
andAssetTransferResult
structs. Consider having different names for different parameters. - Throughout the
PaymentsERC20
contract, the local variable name forPayment
struct isp
.
Consider having a meaningful naming convention for variables instead of using single letters such as p
or abbreviated words such as inp
.
Update: Fixed as of commit 4c2dc66
of PR #7.
Registration process is not clearly documented
The PaymentsERC20
contract allows a seller to register themselves by calling the registerAsSeller
function. However, the system allows purchase of assets from unregistered sellers if the _isSellerRegistrationRequired
flag is not set. In the scenario where _isSellerRegistrationRequired
is set to false, any calls to registerAsSeller
would be meaningless and would result in seller paying for the gas used for making this unnecessary transaction.
The codebase fails to document the meaning of “registration”, the advantage of being a registered seller, and why is it okay to disable this process.
Consider adding proper documentation as to why this process is required in the system and the scenarios in which it can be changed or disabled.
Update: Fixed as of commit 65269d9
of PR #8. However, there is a typographical error in the newly added docstring. In the README
and docstring in the PaymentsERC20
contract, “executed” is misspelled as “exectuted”.
Notes & Additional Information
Constant set by constructor not marked immutable
The PaymentsERC20
contract sets the value of the _erc20
storage variable at construction time and has no mechanism to change the value afterward. This variable can be marked as immutable to save on gas costs when reading the value.
Consider marking this variable immutable
in order to save users gas fees.
Update: Fixed as of commit 0f8028c
of PR #10.
Contract file and contract name do not match
The ERC20Token.sol
file, which is used only for test purposes and is not deployed, contains the MyToken
contract. To follow common coding conventions, consider renaming ERC20Token.sol
to MyToken.sol
so that the contract name and the filename are the same.
Update: Fixed as of commit 2172356
of PR #11.
Use of draft EIP code
The EIP712Verifier
contract uses OpenZeppelin’s implementation of the draft EIP712 standard for hashing and signing of struct data. Until the EIP712 proposal is finalized, it is possible that the standard will change, which could require updates to the EIP712Verifier
contract. Because the PaymentsERC20
contract inherits from EIP712Verifier
, the end result could require a redeployment of the PaymentsERC20
contract which stores payment data and user funds.
Consider refactoring the code to allow EIP712Verifier
to be deployed as a separate contract that could be upgraded in isolation in the event of a change to the draft EIP712 standard.
Update: Fixed as of commit d08c9f0
of PR #26.
Imports not grouped together
The following contracts have a docstring inserted in between the import statements:
EIP712Verifier.sol
– import on line 16 is separated from the imports on lines 4-5IPaymentsERC20.sol
– import on line 41 is separated from the imports on lines 4-6PaymentsERC20.sol
– import on line 41 is separated from the imports on lines 4-6
To favor code clarity, consider rearranging the file contents in each case to group all of the import statements together.
Update: Fixed as of commit 2bba4d4
of PR #13.
Misleading documentation
For clarity, consider revising the following comments which may cause confusion:
In IEIP712Verifier.sol
, lines 10-11 state:
* @dev This contract just defines the structure of a Payment Input
* and exposes a verify function, using the EIP712 code by OpenZeppelin
However, the contract defines two structures and two verify functions, not just PaymentInput
and verifyPayment
as indicated by the docstring.
In PaymentsERC20.sol
, lines 18-20:
* To start a payment, the signatures of both the buyer and the Operator are required.
* - in the 'relayedPay' method, the Operator is the msg.sender, and the buyerSig is provided;
* - in the 'pay' method, the buyer is the msg.sender, and the operatorSig is provided.
This docstring could be misinterpreted to mean that both buyerSig
and operatorSig
are required to start a payment, i.e. that both functions must be called. If buyerSig
is provided, the operator’s signature is provided by signing the relayedPay
transaction, and similarly if operatorSig
is provided, the buyer’s signature is provided by signing the pay
transaction. This comment is also repeated in IPaymentsERC20.sol
and README.md
.
Update: Fixed as of commit f580cf5
of PR #14.
Mismatched parameter name between interface and contract
In the EIP721Verifier
contract, the verifyAssetTransferResult
function declaration has the parameter AssetTransferResult calldata inp
, but the corresponding IEIP712Verifier
interface names the same parameter result
instead of inp
. The @inheritdoc tag used for verifyAssetTransferResult
will inherit an incorrect docstring in this case.
To avoid confusion, consider renaming these parameters so that the names match each other, and match the corresponding docstring @param tag.
Update: Fixed as of commit 4c2dc66
of PR #7.
Typographical errors
Consider addressing the following typographical errors:
In IPaymentsERC20.sol
:
- line 12: “success of failure” should be “success or failure”
- line 16: “all of buyer’s received tokens” should be “all tokens received from the buyer”
- line 24: “has non-zero local balance” should be “has a non-zero local balance”
- line 28: “States Machine” should be “State Machine”
- line 34: “the a payment” should be “the payment”
- line 36: “throught” should be “throughout”
- line 47: “Payin” should be “PayIn”
- line 232: “ot check” should be “to check”
In PaymentsERC20.sol
:
- line 12: “success of failure” should be “success or failure”
- line 16: “all of buyer’s received tokens” should be “all tokens received from the buyer”
- line 24: “has non-zero local balance” should be “has a non-zero local balance”
- line 28: “States Machine” should be “State Machine”
- line 34: “the a payment” should be “the payment”
- line 36: “throught” should be “throughout”
In README.md
:
- line 16: “success of failure” should be “success or failure”
- line 19: “recevied” should be “received”
- line 20: “all of buyer’s received tokens” should be “all tokens received from the buyer”
- line 28: “has non-zero local balance” should be “has a non-zero local balance”
- line 32 and line 46: “States Machine” should be “State Machine”
- line 40: “throught” should be “throughout”
Update: Fixed as of commit fa5cb16
of PR #15.
Unnecessary use of uint16
The PaymentInput
struct defined in the IEIP712Verifier
contract contains a feeBPS
variable whose type is uint16
. This value is used in the _finalizeSuccess
function to determine the transaction fee which is subtracted from the seller’s gross sale proceeds. The feeBPS
value is passed by _finalizeSuccess
to the computeFeeAmount
function, which performs the actual fee calculation.
Although feeBPS
is a uint16
value, the computeFeeAmount
function expects feeBPS
to be a uint256
value, so an explicit cast to uint256
is performed when the function is called by _finalizeSuccess
. Since feeBPS
is converted to a uint256
type at the point of use, and the variable is not being packed in the PaymentInput
struct, it is unnecessary to store the value in a uint16
type.
Consider changing the feeBPS
variable type from uint16
to uint256
.
Update: Fixed as of commit 3e21d35
of PR #17.
Unnecessary inheritance
The FeesCollectors
contract inherits from the Operators
contract, effectively concatenating the two independent contracts. These contracts do not share any state or functions that require the use of inheritance.
To favor clarity in the code hierarchy, consider removing the inheritance relationship in this case, allowing FeesCollectors
to independently inherit from Ownable
, and PaymentsERC20
to inherit from both FeesCollectors
and Operators
rather than just FeesCollectors
.
Update: Fixed as of commit cf5e0ea
of PR #18.
Unused imports
To improve readability and avoid confusion, consider removing the following unused imports:
In the IPaymentsERC20
contract:
In the IEIP712Verifier
contract:
Update: Fixed as of commit b633fdc
of PR #19.
No functionality to withdraw portion of funds
The current system does not allow the users to take out a portion of their funds. The funds can be withdrawn from the PaymentsERC20
contract by calling either the finalizeAndWithdraw
, refundAndWithdraw
or the withdraw
functions which in-turn calls the private _withdraw
function. This _withdraw
function checks the balance of the caller and transfers the entire balance to the caller’s address.
Although there are no security concerns with withdrawing the entire balance, it might be advisable to have an option of withdrawing a portion of funds to provide a better user experience. For example, if a user has made a profit on selling an asset, they might want to withdraw only the profitable amount and reuse the remaining funds for future trades. In the current scenario, the user would be forced to take out all of their balance and then transfer funds back to the system in order to engage with future trades, and subsequently pay for the gas for making the additional transaction.
Consider adding functionality that allows the users to withdraw a portion of their funds.
Update: Fixed as of commit 20af988
of PR #23.
Conclusions
1 critical severity issue was found. Some changes were proposed to follow best practices and reduce the potential attack surface.