Endaoment team asked us to review and audit their smart contracts. We looked at the code and now publish our results.
We audited commit
f60aa253d3d869ad6460877f23e6092acb313add of the endaoment/endaoment-contracts repository. The scope includes all the smart contracts in the repository, except the mocks used for testing.
Endaoment project is a Donor-Advised Funds platform, where everyone is free to transfer their
ERC20 tokens to a
Fund contract to fund approved organizations with the use of grants. More grants can be created for the same fund and when a specific grant is finalized an amount of donated tokens is transferred to the recipient organization. The funds received by an organization from the grant can be then cashed out.
Anyone can propose claims containing basic information about an organization but an administrative governance choses which claims to approve, which organizations to allow and which grants can be finalized. The entire system is governed by owners of the
EndaomentAdmin contract which stores and provides specific role accesses to users interacting with the system. They also manage two factories to deploy
Org contracts, corresponding to allowed organizations, and
EndaomentAdmin contract is the one in charge of managing all restricted accesses to functions, included the ones to move tokens from
Fund contracts to
Org contracts and to cash them out.
The creator of this contract can transfer their ownership with a two-step transfer and is in charge of setting and revoking the following roles to the corresponding addresses:
PAUSER: to pause other roles and disable them temporary. Unpause them is an exclusive right of the contract
ACCOUNTANT: to finalize grants, cash out organizations and to create
REVIEWER: to change the wallet associated to an organization and overwrite the fund’s manager address.
ORG_FACTORY: to deploy new
ADMIN: to have full control over the system and the exclusive role of deploying new factories.
Fund contract has a specific fund manager that can create new grants.
A two-step transfer of ownership is, in general, a safe pattern to follow, but users must be aware of the fact that a transfer is not completed when initialized. Instead, this is completed when the new
owner accepts the transfer. If an
owner is trying to cancel the ownership transfer, it can be front-runned by the recipient owner trying to mine a transaction that accepts the transfer before the cancellation happens.
Overall the code is extensively commented and easy to understand with very well encapsulated contracts and functions. However, our main concern are the vulnerabilities we found in the roles implementation that can lead to loss of funds. The privileged roles built in the system are in general a safe way to protect access to funds management, but they should be finely implemented to avoid any possible attack from malicious actors. Also the lack of technical documentation makes it hard to understand the exact purpose and functionality of each one of the contracts, and thus makes them hard to audit.
Since fixing the issues we have found would require a lot of changes in the code, consider improving the tests with a particular focus on adversarial scenarios, and auditing again the code once updated.
Update: The Endaoment team did a great job reviewing the code, refactoring when necessary and fixing all the major issues. The fixes changed many lines of code, but the governance model has still the same overall architecture. We are happy to see that a very detailed and complete documentation has been created and made available for users and developers willing to learn more about the system. Even if all major issues have been fixed, the changes done produced a completely different code base which may benefit from another audit process, perhaps with a restricted scope made out of the contracts that had more changes. We recommend the Endaoment team to do extensive beta testing on their system before releasing it to production.
[C01] Governance restrictions can be bypassed
Update: Fixed in pull requests #76 and #77. The Endaoment team created a new
EndaomentAdminStorage contract that keeps track of the current
EndaomentAdmin implementation saved in the
endaomentAdmin parameter. This contract extends from the
Administratable one and also provides a restricted method to eventually change the address of the implementation. The factories now extend from such contract and their addresses are stored in the
Org contract whenever a new one is created. In this way the
endaomentAdmin of both factories is passed as input parameter in the
Administratable modifiers whenever a restricted function of the
Org contract is called. It has to be noticed that, before interacting with the system, the correct roles must be set in the
EndaomentAdmin contract configured in the
EndaomentAdminStorage one. Even if this fixes the issue, an address is still passed as input parameter in the
Administratable modifiers and a new contract has been generated, slightly increasing the gas cost.
The entire system relies on the
EndaomentAdmin contract to restrict access to functions and to separate user roles.
The creator of the
EndaomentAdmin contract is the one in charge of setting specific roles to addresses.
OrgFactory contracts all extend from the
Administratable contract defines the
onlyAdminOrRole modifiers, used to restrict function calls in the derived contracts.
These modifiers receive the
adminContractAddress as input parameter and use it to instantiate the governance contract
They then ask to the
EndaomentAdmin contract at the specified address if the
msg.sender has the proper rights and it eventually reverts if it’s not the case.
The issue is that all the functions making use of these modifiers are
public and receive an arbitrary
adminContractAddress as input parameter.
Functions restricted by the
onlyAdminOrRole modifier are:
onlyAdmin modifier is exclusively used to protect
This opens the doors for the following attack:
- Alice deploys her own
EndaomentAdmincontract and sets her address as having full access (i.e. by setting every role to her address).
- She can now call any of the restricted functions listed above or deploy new factories, passing the address of her malicious
adminContractAddressin the function call parameters.
There are several ways now Alice can benefit from this scenario by just using the malicious
Orgcontract instance, if it has been deployed by the
OrgFactory, would result in an
createGrantfunction of the
Fundcontract checks allowed organizations calling the
checkRecipientfunction. Alice can:
- Create a malicious
Orgcontract by calling the
createOrgfunction of the
OrgFactorycontract making it an allowed organization.
- Change the manager of a
Fundcontract setting her address instead, by calling the
- Call the
createGrantfunction which has the
restrictedmodifier ensuring that the
msg.sender == manager. She will bypass the
requirestatement in line
101and successfully create a grant to her malicious organization.
She can also finalize any grant moving out of the contract the funds needed to cover it by calling the
finalizeGrantfunction. Together with the previous scenario, she can definitely steal all the funds from the contract.
- Once every fund is transferred out to the malicious organization she can cash out everything by calling the
cashOutOrgfunction of the
- She can finally change the
orgWalletof any of the deployed
Orgcontracts by calling the
setOrgWalletfunction or approve any arbitrary malicious claim by calling the
Moreover, all the constructors of the cited contracts are protected by both modifiers, where the
adminContractAddress is passed as constructor parameter. Since these modifiers can be skipped, the constructors of the factories and of
Org contracts can be easily called.
To solve the issue consider doing the following:
- Set the current implementation of the
EndaomentAdmincontract in the constructor of the
Administratablecontract whenever it is called by the derived contract constructors. Save it as state variable.
- Remove the
adminContractAddressinput parameter from any function making use of it and from the modifier definitions.
- Change the modifier implementations to ask directly to the previously set
- Implement an auxiliary function, with restricted access, to change, eventually, the address of the correct
[C02] Fees are wrongly calculated
Update: Partially fixed in pull request #64. Fees can be still evaluated to
0 but the Endaoment team acknowledged this, given the fact that it’s an uncommon outcome and that
grant values are bounded by the web application.
finalizeGrant function of the
Fund contract, the
fee parameter is calculated as
grant.value/100. The entire system assumes that fees are paid to the admin of the fund whenever a grant is finalized.
Due to the Solidity truncating rule for division, if
grant.value < 100, then
fee will be zero and nothing will be paid to the
The rounding error introduced when dividing will always round down the fees paid to the
Consider handling the rounding errors using a library for decimals or introducing specific logic to handle them. Moreover, consider calculating the
finalGrant variable as the
grant.value - fee. This would make consistent the fact that both, if summed up, equal the
ERC20 tokens can be stuck in the contract
Update: Fixed in pull request #68.
Contract interfaces are contracts that declare function signatures without implementing them. They are used as an specification that decouples the signature of the functions from their implementation, allowing to interoperate with different implementations as long as they adhere to the specification.
In the codebase, the
ERC20 token implementation by OpenZeppelin is used in the
Org contracts, instead of using the interface.
The address of the
ERC20 token to be used is passed as input parameter.
Given the fact that one can pass arbitrary addresses whether or not they represent valid
ERC20 tokens and that not every
ERC20 contract is implemented exactly as the OpenZeppelin one, it can happen that transactions calling those functions fail due to a mismatch between the
ERC20 OpenZeppelin contract and the target implementation represented by the
Another consequence is that, since anyone can transfer arbitrary
ERC20 tokens to the
Org contract, if someone accidentally deposits an
ERC20 token which has a different implementation from the OpenZeppelin one, those funds are stuck forever in the contract and can’t be transferred out.
As suggested in [N03], consider using the
IERC20 interface importing it from the
openzeppelin-contracts package to allow interoperation with any possible implementation of the
ERC20 token standard and avoid the loss of the funds deposited in the contracts.
[H02] Not following the Checks-Effects-Interactions pattern
Update: Fixed in pull request #63.
Solidity recommends the usage of the Check-Effects-Interaction Pattern to avoid potential security issues, such as reentrancy.
finalizeGrant function can be used to conduct a reentrancy attack, where the
token transfer in line 129 can call back again the same function, sending to the
admin multiple times an amount of
fee, before setting the
grant as completed.
In this way the
grant.recipient can receive less than expected and the contract funds can be drained unexpectedly leading to an unwanted loss of funds.
Consider always following the “Check-Effects-Interactions” pattern, thus modifying the contract’s state before making any external call to other contracts.
[H03] Token transfers can silently fail
Update: Fixed in pull request #69.
In both cases, the
balanceOf(sender) is not checked to be greater than
0 or greater or equal than the amount that is going to be transferred to the recipient.
Moreover, the returned values of the
transfer methods, if present, are not checked.
To avoid finalizing grants without actually transferring tokens to the recipient organization, consider always checking the result of a token transfer and to validate first the amount to be transferred. This could also save some gas by not emitting unnecessary events.
[M01] Lack of input validation
Update: Fixed in pull request #74.
There is a general lack of input validation in the entire code base. Some examples are:
Fundcontract and the
Orgcontract, are not checking whether the addresses passed as input parameter are the zero address.
createGrantfunction of the
Fundcontract doesn’t check any of the grant input parameters before creating a
grantand push it into the
grantsarray. Only the recipient is checked to be an allowed organization.
- The constructor of the
Orgcontract is not checking if the passed
einvalue is zero.
claimRequestfunction of the
Orgcontract is not validating if the
approveClaimfunction of the
Orgcontract and the
Fundcontract are not checking if the
indexparameter is accessing to a non existing element in the corresponding array.
The lack of validation on user-controlled parameters may result in erroneous or failing transactions. Note also that some user interfaces may default to sending null parameters if none are specified.
Consider reviewing the entire code base and add input validations, especially when an user-controlled input parameter is used to store a value in the contracts.
[M02] Non flexible data objects
Update: Fixed in pull request #75.
Some data objects used in the code like the
grants of the
Fund contract, the
allowedOrgs of the
OrgFactory contract, or the
claims of the
Org contract are all lacking of auxiliary functions to modify their elements after creation.
In particular, allowed organizations cant be disallowed, grants can’t be modified nor deleted after being finalized, and organization claims can’t be modified.
grants have no protection from duplicates when new elements are added.
In order to handle unexpected errors and increase the flexibility of the system, consider adding auxiliary functions, with the proper access restriction, to modify and delete, when necessary, such objects from the arrays or mappings.
Also consider adding a duplicate protection to the arrays using a key-value mapping to rapidly check if an element has been already added or not.
[L01] Missing error messages
Update: Fixed in pull request #53.
There are several require statements in the
EndaomentAdmin contracts without error messages. In particular:
Consider including specific and informative error messages in all require statements.
[L02] README file is missing important information
Update: Fixed in pull request #60.
The README.md of the Endaoment project has little information about what is the purpose of the project nor how to use it. README files on the root of git repositories are the first documents that most developers often read, so they should be complete, clear, concise, and accurate.
Consider following Standard Readme to define the structure and contents for the README file.
Also, consider including an explanation of the core concepts of the repository, the usage workflows, the public APIs and how the code relates to other key components of the project.
Furthermore, it is highly advisable to include instructions for the responsible disclosure of any security vulnerabilities found in the project.
[L03] Missing docstrings
Update: Fixed in pull request #47. Moreover some of the mentioned functions are not existing anymore or they have been refactored.
This hinders 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.
Consider thoroughly documenting the missing functions (and their parameters) that are part of the contracts’ public API.
SafeMath library is not used
Update: Fixed in pull request #54.
To avoid any possible underflow/overflow or division by zero, consider always relying on the proven safety of the
SafeMath library by OpenZeppelin and using it when performing mathematical operations. Also consider removing line 19 of the
Org contract if the contract doesn’t need to perform calculations.
[L05] Misleading docstrings
Update: Partially fixed in pull request #78. Most of the docstrings have been fixed, some of them have been added or refactored.
Several docstrings throughout the code base were found to contain misleading information and should be fixed. In particular:
- Line 16 of the
Fundcontract is incorrectly assuming that the token transfer is done using the
SafeMathlibrary, but since the token address used to do the transfer is arbitrary, then the
transferimplementation can be different and then the assumption doesn’t hold.
- Lines 14,15 and 16 of the
FundFactorycontract state that it provides a way for fetching individual
Orgcontract addresses and a list of
allowedOrgsbut there is no way of retrieving such information from the contract.
- Lines 231 and 232 of the
EndaomentAdmincontract are stating that the permitted roles to pass the modifier’s restrictions have to be a “bot commander (0)” or a “pauser (1)”. In reality, the modifier can be called with any of the six roles permitted and it only checks if that role exists or if it’s paused. Moreover it’s not clear what “bot commander” means.
- Line 182, 172 and 158 of the
EndaomentAdmincontract say “External” when the function visibility is
- Lines 27 an 28 of the
Administratablecontract, the comment says that the only admitted roles are
REVIEWERwhile the modifier actually checks also for the
- Line 184 of the
adminwhere it should be the
roleAddressas the returned named parameter. The same happens in the
- Line 79 and 116 of the
Fundcontract and line 88 of the
Orgcontract are mentioning stablecoins. There is actually no restriction on the type of
ERC20tokens that can be passed as input parameter in the functions making use of them and any mention should therefore be removed.
[L06] Multiple getters for the same state variable
Update: Fixed in pull request #73.
Some contracts in the code base contain multiple public getter functions for the same state variable.
- In the
FundFactorycontract, the automatically generated getter for the
createdFundsarray is duplicated by the explicit
- In the
OrgFactorycontract, the automatically generated getters for the
allowedOrgsmapping and the
deployedOrgsarray are duplicated by the explicit
To avoid duplication, ensure that there is at most one publicly exposed getter for each contract state variable. Consider making all the state variables private, following the style of the openzeppelin-contracts package.
[L07] Useless input parameters
Update: Fixed in pull request #66.
claimRequest function of the
Org contract, the
orgAdminAddress is passed as input parameter. In line 60 then, the
msg.sender is required to be the same address as the
The same happens with the
fSub parameter which is required, in line 59 to be
The function is
public and anyone can call it. The
msg.sender can freely choose their address as
Given the fact that the
fSub are not used for any other purpose we can conclude that the require statements in line 59 and 60 are useless and so the
fSub arguments. Consider removing them.
[L08] Lack of event emission
Update: Fixed in pull request #52.
finalizeGrant functions of the
Fund contract, the
claimRequest and the
setOrgWallet functions of the
Org contract are not emitting any event after changing the value of state variables.
To easily inform clients about state changes in the contract and to let the applications subscribe to such changes, consider always emitting an event when changing important parameters of each contract.
[L09] Missing getter
Update: Fixed in pull request #55.
_newPotentialOwner private state variable of the
TwoStepsOwnable contract is missing a getter function to read its value.
To have access to this state variable without parsing the contract storage in the blockchain, consider adding a agetter function to retrieve its value.
[L10] Incorrect require statement
Update: Fixed in pull request #56.
Administratable contract the
onlyAdminOrRole modifier in line 48 is requiring the
msg.sender to be the
ADMIN address of the
EndaomentAdmin contract, but this statement is inside an conditional structure with the condition that the
msg.sender is not the
require statement will be never satisfied and will always revert if evaluated.
require statement should reflect that the caller is not the
ADMIN or that the role requested is paused and should therefore be a
revert with an explicative error message if the
else block is evaluated.
Consider refactoring the conditional statements by providing a cleaner structure, and reverting when the caller is accessing to a paused role and it’s not the
Notes & Additional Information
Update: Fixed in pull request #49.
To favor explicitness, all instances of
uint should be declared as
[N02] Unused state variables
Update: Fixed in pull request #46.
[N03] Unused contracts
Update: Fixed in pull request #50.
There are files which are not used at all in the code base:
library/SafeMath.solfile is not imported anywhere. Instead, the
Fundcontract, which is the only one performing mathematical operations, uses the
SafeMathlibrary that automatically comes from the OpenZeppelin
ERC20import in the
The same happens with the
IERC20interface. As before, the
ERC20import in the
Orgcontract is used to call
To improve readability, security and clarity of the code, consider replacing the
ERC20 import with the
IERC20 interface and removing the
SafeMath file, importing it, where needed, directly from the OpenZeppelin contracts.
[N04] Style recommendations not applied
Update: Fixed in pull request #51. Solhint is now used as linter tool.
The style used in the entire code base is following the
Solidity style recommendations, however there are still some parts in the code that have some deviations. In particular:
- Events are not named using the CapWord style. Examples are in
cashOutCompleteevent of the
Orgcontract or in the
fundCreatedevent of the
- There are extraspaces in function definitions between the name and the list of input parameters, but also inside functions implementation or trailing whitespaces at the end of the lines.
- Sometimes, indentation is not correct nor made out of four spaces, like the entire
EndaomentAdmincontract, that uses two spaces for the indentation while the majority of the code base is using four.
- Inconsistent patterns are used to return function parameters, some of them are named and some are not.
Orginput parameter of the
getAllowedOrgfunction of the
OrgFactorycontract should be in mixedCase format.
- Error messages are inconsistent in style since sometimes they are introduced by the contract name and sometimes they are not.
- An extra space should be added in several functions where the list of input parameters is not separated by the body’s brackets of the function.
- Some variables have not explicit and unclear names. Like the
OrgFactorycontract or the
- Some variables have names that can be improved to give more context and consistency:
newOwnerinput parameter of the
transferOwnershipfunction of the
TwoStepOwnablecontract could be changed to
_newPotentialOwnerin line 85 of the
TwoStepOwnablecontract could better reflect that that’s the
creatorinput parameter of the
Fundconstructor can be renamed
_adminto be consistent with the variable that is assigned to.
Taking into consideration how much value a consistent coding style adds to the project’s readability, enforcing a standard coding style with help of linter tools such as Solhint is recommended.
[N05] Incorrect functions visibility
Update: Fixed in pull request #62.
Whenever a function is not being called internally in the code, it can be easily declared as
external, saving also gas. While the entire code base have explicit visibilities for every function, some of them can be changed to be
Examples are the getters in the
OrgFactory contract or each one of the public functions of the
To improve clarity following Solidity recommendations and to better reflect the scope of each function, consider reviewing the visibilities applied.
[N06] Unnecessary import
Update: Fixed in pull request #45.
FundFactory contract, consider removing the import statement for
OrgFactory, as this contract is never used in the contract’s code.
Fund contract can’t receive ethers
Update: Fixed in pull request #61.
Fund contract is in charge of recollecting the funds to later support organizations with the recollected tokens. The
getSummary function is returning the
ETH balance of the contract but the contract has no payable functions to receive them.
Consider whether removing any reference to
ETH if the contract is not intended to use it as a valid currency, or implementing the missing functionality to correctly receive and move them out from the contract.
[N08] Typos in the code
Update: Fixed in pull request #42.
There are some typos to fix in the code base. In particular:
- Line 76 of the
Orgcontract is repeated.
- In line 59 of the
OrgFactorycontract, “is provided” should be “if provided”.
- In line 16 of the
Orgcontract, “direct received” should be “directly receive”.
- In the docstring and in the input parameter of the
cashOutOrgfunction of the
withdrawlwords should be changed to
- In the
Fundcontract the word
reccomendationshould be changed for
recommendationin several lines.
- In the
Administratablecontract in line 8
Provides a of modifiersshould be
Provides two modifiers.
[N09] Unclear function purpose
Update: Fixed in pull request #48.
approveClaim function of the
Org contract is having the only purpose of calling the
setOrgWallet function with the
claim.desiredWallet of the claim chosen by the
index input parameter.
On the other side, the
setOrgWallet function is public but called internally only by the
approveClaim function and it is a one line function setting the
orgWallet state variable to the
providedWallet input parameter.
orgWallet variable is not used anywhere in the code base and it’s not clear what’s its purpose.
If is an explicit design decision, consider commenting on the docstrings why
approveClaim is approving a claim or why
setOrgWallet sets that variable and what’s its purpose.
[N10] Default value as sensible information
Update: Fixed in pull request #44.
In the list of all possible user’s roles in the
IEndaomentAdmin interface, the
ADMIN role is the
0 value of the
enum structure which is the default value of the integer type. Relying the
ADMIN role on such value can be risky and should be avoided since many clients can default to that value if none is provided.
Consider moving the
ADMIN role to the end of the list and to assign to the
0 value an
2 critical and 3 high severity issues were found. Some changes were proposed to follow best practices and reduce potential attack surface.
Update: All the reported issues have been addressed and fixed by the Endaoment team.