Skip to content

Instantly share code, notes, and snippets.

@osarrouy
Last active July 2, 2019 13:37
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save osarrouy/62d03760bbce18a8003a2586894a3bae to your computer and use it in GitHub Desktop.
Save osarrouy/62d03760bbce18a8003a2586894a3bae to your computer and use it in GitHub Desktop.
Apiary Audit Feedback

4 Key Observations/Recommendations

The AragonFundraisingController's purpose is to forward and keep in sync other contracts. This forces it to be in sync with all of the other contracts it interacts with, effectively writing a lot of boilerplate to serve as the interface for the whole application. This is a problem because if some functionality is added or removed in any of the market maker, reserve or tap contracts, the controller needs to be updated. Another different way is to remove the controller altogether and have the web interface call the right contract for each functionality. For the methods that keep the collateral tokens (addCollateralToken) and the beneficiary (updateBeneficiary) in sync, a different contract can be used as a ledger having the collateral token list and the beneficiary available (or even split in 2 contracts: one ledger for collateral tokens and the other for the beneficiary); each of the contracts (market maker, reserve and tap) will be able to retrieve the list of collateral tokens or the beneficiary. Managing the collateral tokens or the beneficiary will be done directly in these contracts.

We won't fix this one because it has been designed on purpose to circumvent the Aragon Client's sandboxing limitations [an app's frontend can only interact with one contract] ...

The price of the BON tokens gets lower with every second because of the Tap withdrawal allowance increasing with the same rate, which decreases the pool balance that is used for the BON price calculation.

This is a feature, not a bug ;) This was included to avoid the price to suddenly fall when the project managers actually withdraw the funds they are allowed to withdraw. It makes more sense for the price to get slowly lower as, whatever happens, the related funds are 'virtually' not in the pool anymore.

6 Issues

6.2 No minting limit in the market maker

If there is no limitation in minting tokens by the market maker contract, it's possible for someone with a lot of ETH (or collateral tokens) to buy a lot of BON tokens and become a decision-making monopolist in Voting(BON) contract. Since Voting(BON) has absolute power over Kernel and ACL, it's possible to steal all the collateral from the system. So the initial investment of an enormous amount of money will pay off fully. Additionally, the attacker can destroy the system and take full control over PRO token.

We discussed this with the team and with think the simplest solution for now would just be to revamp the DAOKit and permissions a little so that only PRO holders [i.e. project managers] can initiate vote in the BON democracry-like voting app. This ways voting would need to be initiated by project managers [which would avoid the whale-taking-control-issue] while still needing to approved by BON holders to pass [which would avoid the project-manager-running-out-with-the-money issue].

6.4 Removing tokens from whitelist

In BancorMarketMaker contract there is only a way to add a new whitelisted token or modify some values related to it. There is no way to remove a token from the whitelist if it's compromised.

There is currently a way to block the token by editing some values related to it using updateCollateralToken, but it's indirect and it also blocks all fundraising system without possibility to return funds that are locked in the previous batches.

Yeah. We already thought about that and this is actually an issue but we left it aside because we can't figure out what to do of the already staked collateral tokens in such a situation... This would maybe require another contract allowing people to trade their bond against the malicious staked tokens at a fix price ...

6.6 Batching-related issues

If some whale buys a huge amount of tokens, it's profitable for anyone to buy tokens also in this batch because there is a very high chance that the price will go up after this batch and the next batch will be with a much higher price. Because of doing so, the price will go even higher.

Yes but if someone buy at the same time than a whale it will pay a higher price [the same price than the whale which is high because we have moved a lot along the curve]. Of course you can have expectations that the price goes up after the whale comes in but you can't be sure ... Also depending on the duration of your batch [1 block, 1 hour, 1 day] it's hard to predict if someone else is not gonna sell a lot in the same, batch, etc. which makes the price highly unpredictable. Right ?

6.7 System can be blocked if maxAccountTokens is low

There is a mechanism that limits token minting in TokenManager. It has a restriction of minting tokens to address A if the eventual balance of address A will be more than maxAccountTokens - a limit defined on initialization of TokenManager.

At initialization the maxAccountTokens for the BON token is set to 0 [i.e. infinity] so it should not be an issue.

6.8 Trader has no control over the price

When someone wants to buy (or sell) BON tokens through the market maker, there is no way to ensure that the price won't go infinitely high (or zero). Therefore, the investor's money may be just lost and there is no way to prevent that from happening.

We are aware of that. Unfortunately we don't see a way to solve this problem on-chain: it would require to maybe kick out a buyer from the batch, then re-compute the price, then maybe kick out someone else, then maybe re-introduce the previous buyer because price has dropped, etc. We can't anticipate on the gas cost of such an operation [depending on the number of buyers / sellers in the batch and the extent of their orders] so we would take the risk for the transaction to fail. On solution that dxDAO is using is to make that computation off-chain but then you loose the decentralized rationale ...

6.12 BON-based democracy has full control of the system including PRO tokens

The system's permissions as it defined in the aragonBlack Fundraising Permission Table is fully controlled by a democracy based on the stake in BON tokens. Since the system is based on aragonOS, anyone who has privileged permissions in ACL and Kernel can control the system. This may become a problem if democracy does not work out as planned and someone takes control over Voting(BON) by having a majority among BON token holders.

See the answer to 6.2 above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment