🔗Smartlink
Smart Contract Audit - Smartlink - Escrow System
Dernière mise à jour
Smart Contract Audit - Smartlink - Escrow System
Dernière mise à jour
| Date | 15/06/2023 |
|---|---|
Auditors | Maxance Laloy, Emilien Leclercq |
Audit Company | euraNov |
This security audit was prepared by euraNov.
We cannot guarantee the absolute absence of issues. The audit's scope is limited to the Solidity code and may not cover other project components or future changes. It also acknowledges that the auditor's liability is limited to the professional services provided during the audit.
euraNov cannot be responsible for errors, omissions, or the direct or indirect consequences, damages, losses, or fraud arising from the use of theses smart contracts.
Type : Escrow
Languages : Solidity
| Issues | Detecred |
|---|---|
Critical Severity | 0 (0 resolved) |
High Severity | 3 (3 resolved) |
Medium Severity | 1 (1 resolved) |
Low Severity | 5 (4 resolved) |
Notes & Additional Information | 16 (14 resolved) |
Total Issues | 25 (22 resolved) |
We audited the Escrow/smart-contract-solidity repository at the 7eddbd35 commit.
In scope were the following contracts for the :
Update 15 juin 2023 : Update of this audit report after incorporating the changes made by the development team (last commit [8cba5854](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/8cba58547d080cdac555ec0cb3a006e70be2ec78)).
Firstly, the Factory smart contract is a simple contract that aims to facilitate and secure the deployment of Escrow smart contracts.
The escrow system is a smart contract designed to facilitate secure transactions between two parties by temporarily holding funds until predefined conditions are met. This escrow system is particularly useful for transactions involving digital assets, services, or high-value goods, providing a trustless mechanism to ensure all parties fulfill their obligations. The Escrow contract is an extension of a simple Escrow smart contract, introducing additional features and improvements over the basic escrow functionality.
The primary difference lies in the inclusion of user-defined release conditions. The escrow system expands upon the traditional releaseFunds function by incorporating a more specific condition-checking mechanism. This mechanism allows users to define custom conditions in the form of a timeline such as withdrawal, delivery, inspection, and extension periods. The release of funds is only authorized when the defined conditions are fulfilled, providing enhanced security and flexibility for users.
Another distinction is the implementation of a transaction tracking system within the contract. The Escrow contract employs a comprehensive transaction status tracking system that monitors the progress of each transaction through different statuses. This system replaces the basic released mapping in a basic Escrow contract. By tracking transaction statuses the escrow system enables additional functionality, such as multi-stage transactions and conditional fund releases. Admin can set predefined milestones for the release of funds, ensuring each stage of a transaction is completed as agreed upon.
Another important and notable aspect is the implementation of an arbitration system that allows an external person to arbitrate in the event of a transaction dispute between the two parties. This arbitrator is pre-defined in the contract (whitelisted) and operates in exchange of arbitration fees.
The main covered aspects are :
The buyer can initiate an escrow transaction by calling the addOrder or addOrderByToken methods on the smart contract.
The seller can cancel the transaction at any time (funds are automatically returned to the buyer) by calling the cancelOrder method.
During the withdrawal period, the buyer can cancel the transaction by calling the cancelOrder method.
During the inspection period:
The buyer can confirm the delivery by calling the completeOrder method.
The seller and buyer can request an extension of the inspection period by calling the extendInspectionRequest method.
The buyer and seller can request a settlement offer by calling the settlementRequest method.
The buyer and seller can initiate an arbitration process by calling the addDispute or addDisputeByToken methods.
After the inspection period, the seller can claim the funds for the transaction by calling the claim method.
A final claim can be made after one year by the admin of the Factory by calling the superClaim method.
In summary, this escrow system offers a secure, flexible, and customizable escrow solution that caters to a diverse range of transaction types and user requirements. It introduces user-defined release conditions and advanced transaction tracking, ensuring a more versatile escrow experience for all parties involved.
Smart contracts can contain vulnerabilities, bugs, or under specific circumstances such as technical issues, it’s essential to be able to temporarily stop the execution of the contract to prevent any further exploitation. The pause function allows developers to react quickly to such situations and protect the involved users and assets. This promotes better risk management and enhances user experience. We strongly recommend implementing a pause function.
Corrected : [3011039f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/3011039f3b1b17533688af66124b061dde088523)
Proper initialization of state variables in a Solidity smart contract is essential to prevent undesired behaviors, ensure security, facilitate interoperability, and improve code readability. This contributes to the contract's robustness, security, and maintainability. For these reasons, it is crucial to correctly initialize the variables Escrow.pause #L25 and Factory.pause #L9 .
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
If we follow the logic, it appears that in the Escrow.sol contract, in the superClaim function, the transfer should be made to the address of the super_admin rather than the admin's address #L507.
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
The changeConfig function allows the admin to update token statuses and the fees_paid_byvalue. However, there is no validation to ensure that the _tokens and _tokenStatus arrays have the same length. If they have different lengths, this can lead to unexpected behavior. You should add a check to ensure that the lengths are equal.
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
In the changeConfig function, the variable _status#L202 has the same name as a variable present in the ReentrancyGuard.sol contract. In Solidity, it is possible to use the same variable name in different scopes; however, this can potentially lead to undesirable effects. To improve readability and minimize errors, we recommend renaming the variable to _tokensStatus.
Corrected : [fd2f4254](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/fd2f4254932ae3ef2509c4ef705c194ddacfa876)
In the addOrderByToken function, even though a reentrancy attack has no interest in this function, it is still preferable to follow the checks-effects-interactions pattern to maintain consistency in the code and avoid any potential attack vectors.
After discussing with the development team, this issue will not be taken into account and is therefore considered resolved.
By checking addresses are not equal to address 0, we avoid transfer errors to an invalid address and prevent the loss of funds. Furthermore, an invalid address such as address 0 can be exploited by attackers to manipulate the smart contract's functionality. It’s important to verify that addresses are not equal to address 0 in order to ensure data validity, security, and enhance the contract's robustness.
Escrow.constructor :
admin = _admin #L92
affiliation_address = _affiliation_address #L101
fee_collector = _fee_collector #L94
arbitrator_address = _arbitrator_address #L93
Escrow.changeAffiliate function :
affiliation_address = _address #L191
Escrow.changeFeeCollector function :
fee_collector = _address #L195
Escrow.changeSuperAdmin function :
super_admin = _address #L183
Escrow.changeArbitrator function :
arbitrator_address = _address #L187
Factory.constructor :
Factory.changeArbitrator :
arbitrator_address = _address #L63
Factory.changeAdmin :
admin = _address #L40
Factory.changeFeeCollector :
fee_collector = _address #L43
After discussing with the development team, adding verification in correlation with the other modifications would have resulted in excessive contract size. In the interest of optimization, this issue is therefore not resolved but also not a security issue here.
The initialization of the counter variable before the start of the loop ensures that its value is correct from the beginning, thus avoiding unexpected behaviors or incorrect calculations. As best practice, we recommend to properly initializing the i variable in the Escrow.addToken function #L158.
After discussing with the development team, due to the fact that a `uint` type variable is initialized by default to the value of 0, this issue has been decided to be marked as solved.
When performing a transfer or any other interaction with an external smart contract, it is preferable to use an IERC20 interface to interact with it for several reasons. Using an IERC20 interface ensures clarity and readability of the code by concisely declaring the specific functions and events of an ERC20 contract. Additionally, it provides enhanced security and reusability by utilizing standardized functions proven by the community. The use of an IERC20 interface is recommended to improves code quality, security, maintainability, and interoperability of the smart contract.
Factory.recoverTokens #L68
Escrow.addOrderByToken #L223
Escrow.cancelOrder #L285
Escrow.completeOrder #L313
Escrow.completeOrder #L316
Escrow.completeOrder #L319
Escrow.acceptSettlementRequest #L370
Escrow.acceptSettlementRequest #L373
Escrow.acceptSettlementRequest #L376
Escrow.acceptSettlementRequest #L379
Escrow.addDisputeByToken #L438
Escrow.addDisputeByToken #L440
Escrow.claim #L478
Escrow.claim #L481
Escrow.claim #L484
Escrow.superClaim #L509
The settlementRequest function allows the buyer or the admin to request a settlement for the order, but it does not have a check to ensure that the _percentage argument is within a valid range.
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
In these two functions addDisputeByToken and addOrderByToken, the non-reentrancy modifier has been added, however the checks-effects-interactions pattern is not followed. Even though the modifier serves as a security measure, it is still preferable to adhere to this pattern to minimize the potential surface for a reentrancy attack.
After discussing with the development team, the non-reentrancy modifier is deemed sufficient. Therefore, this issue is considered solved.
Throughout the Escrow.sol smart contract, you use different naming conventions, such as snake_case and camelCase (for example: extensionRequests #L60, order_ids #L57). For better code readability and maintainability, its preferable to use a single and consistent naming convention across all smart contracts.
After discussing with the development team, this issue is considered resolved.
Immutable variables cannot be modified after their initialization. This ensures their integrity and prevents potential errors from manipulation or accidental modification. Additionally, immutable variables are optimized by the compiler, which can improve performance and reduce gas costs during contract deployment and execution. We recommend declaring the following variables as immutable:
Escrow.delivery_period #L18
Escrow.extension_period #L20
Escrow.request_reply_period #L21
Escrow.admin #L15
Escrow.factory_contract_address #L16
Escrow.withdrawal_period #L17
Escrow.inspection_period #L19
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
require statements use short error codes, more descriptive error messages can improve readability and debugging.
After discussing with the development team, with a focus on optimizing contract size, the team has chosen to keep short error codes. A comprehensive documentation will be provided for users. This issue is therefore considered resolved.
The Escrow.sol contract does not have any built-in upgradability features. If you anticipate needing to make changes to the contract's logic after deployment, consider implementing a proxy pattern or using a more modular design that allows individual components to be upgraded.
After discussing with the project team, the topic of contract update was not included in this version. Therefore, this issue is not considered resolved.
The Escrow.sol & Factory.sol smart contracts does not provide any documentation. Ensure that the smart contract provide clear and comprehensive documentation to guide users and developers on how to interact with the contract.
Corrected : The development team has chosen to place the documentation outside the smart contracts.
Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Replace pragma solidity ^0.8.17; by pragma solidity 0.8.17; Escrow.sol #L2/Factory.sol #L2.
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
You can replace the following functions with modifier to ensure consistency, improved readability, and enhanced maintainability of the code :
Partially corrected : [8b2dc676](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/8b2dc6766c7cc24de075f31031f82e558411e392) For optimization purposes, the functions `Escrow.validOrder` and `Escrow.validSettlement` have been removed. The function `Escrow.validStatus` has not been modified.
Events provide transparency and traceability by recording important actions on the blockchain. They allow users, third-party applications, and contracts to monitor and react to changes in the contract's state.
We recommend adding the emission of an event for each of the following functions :
Escrow.changeSuperAdmin #L182
Escrow.changeArbitrator #L186
Escrow.changeAffiliate #L190
Escrow.changeFeeCollector #L194
Escrow.changeSuperAdminClaimPeriod #L198
Factory.changeAdmin #L39
Factory.changeFeeCollector #L42
Factory.changeArbitrator #L62
After discussing with the project team, emitting events for these functions was deemed unnecessary. Therefore, this issue is considered resolved.
The token information is stored as a copy during the deployment of an Escrow.sol contract :
The status can be modified independently of the Factory using the changeConfig function, which is accessible only to the Escrow contract administrator. If desired, in the case where a token is blacklisted by modifying its status in the Factory, this status change will not be automatically reflected in the already deployed Escrow contracts.
Once the token fees have been initialized at instantiation, it is not possible to update them from the Escrow. Only the Factory can update the fees, and the new configuration will be used only for deploying future Escrow contracts.
Adding a new token is possible through the changeConfig function, which verifies in the Factory that the token's status is valid. This means that for each token added to the Factory, as long as the administrators of the Escrow contracts have not called the changeConfig function, the token will not be available in their Escrow contract.
After discussing with the project team as well as the development team, the chosen behavior is intentional. Therefore, this issue is considered resolved.
The Escrow.Status.SR (SETTLEMENT_REQUEST) status is not defined in the order statuses when calling the Escrow.settlementRequest function.
The Escrow.Status.NA status is not used #L63.
Corrected : [94582fb6](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/94582fb6861a4a32283afabe2b04e0ce14298c24)
The configuration functions in the Escrow contract (only the Factory admin), must be called on each deployed Escrow contract in case of modification : Escrow.changeSuperAdmin, Escrow.changeArbitrator, Escrow.changeAffiliate, Escrow.changeFeeCollector, Escrow.changeSuperAdminClaimPeriod.
The same goes for the Escrow.superClaim function, but it is less sensitive as it is less likely to be called.
After discussing with the project team as well as the development team, the chosen behavior is intentional. Therefore, this issue is considered resolved.
The times #L90 parameter is not clearly defined, and index 5 of this parameter is used for fees_paid_by, which does not correspond to a time period. One solution could be to separate the fees_paid_by parameters from the time periods or rename the times parameter to options to avoid any confusion. This would make the code clearer and easier to understand for developers and users of the escrow contract.
Corrected : [1446763f](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/1446763fdd391415b56ad44d1f953b2feddf80fe)
The Factory.sol & Escrow.sol contract don't require to receive standalone Ether payments or perform any action when it receives Ether, you don't need to explicitly include the receive function.
On the other hand, the fallback function i used to handle function calls that didn't match any other defined function in the contract. It is considered good practice to define a fallback function that reverts in case of function calls that didn't match any other defined function in the contract. This ensures that any unexpected or invalid function calls to your contract are handled properly.
It has been chosen to keep the `receive` function so that the super admin can manually perform the reverse transfer in case of ETH transfer by mistake on the `Escrow.sol` and `Factory.sol` contracts. Corrected : [305aeb41](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/305aeb416d1e0eb12dcab11668b22e7e710b4bbd)
The following tests failed during our execution :
Factory.DeployEscrow #L197 : “Should deploy escrow with new address”
Factory.DeployEscrow #L209 : “Should deploy escrow having two token”
Corrected : [8b2dc676](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/8b2dc6766c7cc24de075f31031f82e558411e392)
L’équipe de développement à effectué un nombre notable de modification dans le but d’optimiser la taille du smart contract.
Corrected : [4ef1845c](https://gitlab.com/smartlink-escrow/smart-contract-solidity/-/tree/4ef1845c42b47fde7d21f90e83da5728281c5a4b)
In conclusion, the Factory.sol contract doesn’t contain any major issues, only a few minor improvements could be made.
On the other hand, the Escorw.sol contract seems to have three high-severity issues. These issues have been processed. Besides this, the contract is using an up-to-date and appropriate version of the Solidity compiler which includes many safety features.
Visibility specifiers are well defined in all functions which is a good practice and can reduce the chance of unintentional behavior. The contract uses modifiers correctly to handle access control and checks for conditions before execution of function bodies. This helps in reducing code repetition and improving readability.
Revert statements with error messages are used in the contract which is a good practice for debugging and understanding why a transaction might have failed. But it is necessary to provide more explicit error messages and/or comprehensive documentation to clearly and easily identify the errors returned by the smart contract.
Given these insights, measures have been taken to prevent any potential reentrancy attack. The "Checks-Effects-Interactions" pattern has been mostly adhered to.
Additionally, to ensure the logic of the contract works as intended, further testing should be conducted, including unit testing, integration testing, and stress testing.
Due to the weight of the `Escrow.sol` smart contract code, some improvements and/or optimizations regarding the code and gas consumption are currently not possible. The development team has already implemented some optimizations; however, it is important to consider this aspect in the context of future improvement and optimization efforts to ensure the best user experience.
euraNov - 15 juin 2023
