| Audit Date | 01/02/2023 - 01/03/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 01/03/2023 Initial Report |
| Version 2 | 01/08/2023 Mitigation Review |
- Audit Report - PartyDAO
- Contents
- Disclaimer
- About HollaDieWaldfee
- Scope
- Severity Classification
- Summary
- Findings
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contract, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top ranked Smart Contract Auditor doing audits on code4rena (www.code4rena.com) and Sherlock (www.sherlock.xyz), having ranked 1st in multiple contests.
On Sherlock he uses the handle "roguereddwarf" to compete in contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter: @HollaWaldfee100
The scope of the audit is the following Pull Request in the client's GitHub repository: PartyDAO/party-protocol#360
Specifically, the Pull Request adds the BondingCurveAuthority contract to the protocol which allows users to buy and sell party cards based on the bonding curve price.
The commit for the initial audit is b2e4ec23d0aea149f66aeabd157838f5e7f7cb2b.
The commit for the mitigation review is a1e09805f711aac58199e1e2a074aed4a75f18e0.
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
| Severity | Total | Fixed | Partially Fixed | Acknowledged | Disputed | Reported |
|---|---|---|---|---|---|---|
|
1 | 1 | 0 | 0 | 0 | 0 |
|
2 | 1 | 1 | 0 | 0 | 0 |
|
3 | 2 | 0 | 1 | 0 | 0 |
|
3 | 3 | 0 | 0 | 0 | 0 |
Description:
An arbitrary partyFactory address can be provided to the BondingCurveAuthority.createParty() and BondingCurve.createPartyWithMetadata() function.
A malicious contract can be provided instead of the legitimate partyFactory that returns the address of an existing party that has set the BondingCurveAuthority as its authority.
This allows an attacker to set the PartyInfo for the existing party (Link).
Impact:
An attacker can set the PartyInfo for any party and so he can essentially mint party cards for free by setting the appropriate values for a and b.
By doing so, he can gain as much voting power as he wants for any party that was created with the BondingCurveAuthority contract.
The attacker can thus take over the party including any tokens within it.
Recommendation:
The partyFactory must be a trusted contract which ideally is immutable and set upon construction of the BondingCurveAuthority:
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..63d2683 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -58,6 +58,7 @@ contract BondingCurveAuthority {
/// @notice The intrinsic voting power of party cards minted by this contract
uint96 private constant PARTY_CARD_VOTING_POWER = uint96(0.1 ether);
address payable private immutable PARTY_DAO;
+ address private immutable PARTY_FACTORY;
uint16 private constant BPS = 10_000;
uint16 private constant MAX_CREATOR_FEE = 250; // 2.5%
uint16 private constant MAX_TREASURY_FEE = 1000; // 10%
@@ -67,8 +68,6 @@ contract BondingCurveAuthority {
/// @notice Struct containing options for creating a party
struct BondingCurvePartyOptions {
- // The party factory address to use
- PartyFactory partyFactory;
// The party implementation address to use
Party partyImpl;
// Options for the party. See `Party.sol` for more info
@@ -110,7 +109,8 @@ contract BondingCurveAuthority {
address payable partyDao,
uint16 initialPartyDaoFeeBps,
uint16 initialTreasuryFeeBps,
- uint16 initialCreatorFeeBps
+ uint16 initialCreatorFeeBps,
+ address partyFactory
) {
if (initialPartyDaoFeeBps > MAX_PARTY_DAO_FEE) {
revert InvalidPartyDaoFee();
@@ -125,6 +125,7 @@ contract BondingCurveAuthority {
treasuryFeeBps = initialTreasuryFeeBps;
creatorFeeBps = initialCreatorFeeBps;
PARTY_DAO = partyDao;
+ PARTY_FACTORY = partyFactory;
}
/**
@@ -142,7 +143,7 @@ contract BondingCurveAuthority {
_validateGovernanceOpts(partyOpts.opts);
- party = partyOpts.partyFactory.createParty(
+ party = PartyFactory(PARTY_FACTORY).createParty(
partyOpts.partyImpl,
authorities,
partyOpts.opts,
@@ -181,7 +182,7 @@ contract BondingCurveAuthority {
_validateGovernanceOpts(partyOpts.opts);
- party = partyOpts.partyFactory.createPartyWithMetadata(
+ party = PartyFactory(PARTY_FACTORY).createPartyWithMetadata(
partyOpts.partyImpl,
authorities,
partyOpts.opts,It is also possible to introduce a setter function for the party factory which only the PARTY_DAO address can call.
However, this introdues additional trust assumptions and the immutable party factory is the preferred solution.
Mitigation Review:
The issue has been fixed by implementing a zero address check in createParty() and createPartyWithMetadata():
if (partyInfos[party].creator != address(0)) {
revert ExistingParty();
}This fix, other than the recommendation, allows the BondingCurveAuthority to work with any partyFactory.
If a malicious partyFactory would return the address of a Party that has already been created with the BondingCurveAuthority, the new check would cause a revert.
And if the malicious partyFactory returns the address of a Party that has not yet been created with the legitimate factory in an attempt to cause a DoS, the subsequent calls to e.g., Party.increaseTotalVotingPower() would revert.
In conclusion, the attack has been mitigated and the mitigation does not introduce any new attack surface.
Description:
Parties can create distributions to distribute tokens within the party to its members according to their share of the total voting power.
The issue is that by introducing authorities that can increase and decrease the total voting power and the voting power of individual party cards, the logic in the TokenDistributor is broken.
The TokenDistributor allows users to claim their share of the funds if the following condition is met:
if (
partyTokenId > maxTokenId &&
info.party.getGovernanceValues().totalVotingPower > info.totalShares
) {
revert TokenIdAboveMaxError(partyTokenId, maxTokenId);
}By flash minting party cards via the BondingCurveAuthority contract, a user can get access to new party cards that haven't claimed their share yet without increasing the totalVotingPower.
As a result, a malicious user can claim the whole distribution for himself.
Impact:
As described above, once a distribution is created, a malicious user can claim all the distributed funds, stealing them from other party members.
There currently exists the distributionsRequireVote party setting that, if set to true, at least requires that a distribution must be proposed and voted on.
Since all distributions are unsafe if the party has set the BondingCurveAuthority as an authority, hosts must come in and veto the proposal within the executionDelay period.
If we assume that distributionsRequireVote is always set to true and executionDelay is always big enough for hosts to veto proposals, this issue can't occur.
However, since distributionsRequireVote = true and a safe executionDelay are currently not enforced and distributions have only become dangerous now that authorities have been introduced, this issue is of "Medium" severity.
Recommendation:
Simply setting distributionsRequireVote = true is insufficient since all distributions are unsafe, even if they are voted for.
So in addition to the recommendations in issue #6, it should be possible to completely disable distributions within a party.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..128e004 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -20,6 +20,7 @@ contract BondingCurveAuthority {
error ExecutionDelayTooShort();
error EthTransferFailed();
error ExcessSlippage();
+ error DistributionsNotAllowed();
event TreasuryFeeUpdated(uint16 previousTreasuryFee, uint16 newTreasuryFee);
event PartyDaoFeeUpdated(uint16 previousPartyDaoFee, uint16 newPartyDaoFee);
@@ -214,6 +215,10 @@ contract BondingCurveAuthority {
if (partyOpts.governance.executionDelay < MIN_EXECUTION_DELAY) {
revert ExecutionDelayTooShort();
}
+
+ if (partyOpts.proposalEngine.distributionsFlag != 2) {
+ revert DistributionsNotAllowed();
+ }
}
/**
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol
index b6a74c6..096171e 100644
--- a/contracts/party/PartyGovernance.sol
+++ b/contracts/party/PartyGovernance.sol
@@ -190,6 +190,7 @@ abstract contract PartyGovernance is
error InvalidBpsError(uint16 bps);
error InvalidGovernanceParameter(uint256 value);
error DistributionsRequireVoteError();
+ error DistributionsNotAllowedError();
error PartyNotStartedError();
error CannotModifyTotalVotingPowerAndAcceptError();
error TooManyHosts();
@@ -492,13 +493,16 @@ abstract contract PartyGovernance is
uint256 tokenId
) external returns (ITokenDistributor.DistributionInfo memory distInfo) {
_assertNotGloballyDisabled();
+ if (_getSharedProposalStorage().opts.distributionsFlag == 2) {
+ revert DistributionsNotAllowedError();
+ }
// Ignore if the party is calling functions on itself, like with
// `FractionalizeProposal` and `DistributionProposal`.
if (msg.sender != address(this)) {
// Must not require a vote to create a distribution, otherwise
// distributions can only be created through a distribution
// proposal.
- if (_getSharedProposalStorage().opts.distributionsRequireVote) {
+ if (_getSharedProposalStorage().opts.distributionsFlag == 1) {
revert DistributionsRequireVoteError();
}
// Must be an active member.
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index 4bbbbdf..27bd5c9 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -99,7 +99,7 @@ abstract contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
name = name_;
symbol = symbol_;
if (rageQuitTimestamp_ != 0) {
- if (!proposalEngineOpts.distributionsRequireVote) {
+ if (proposalEngineOpts.distributionsFlag == 0) {
revert CannotEnableRageQuitIfNotDistributionsRequireVoteError();
}
@@ -346,7 +346,7 @@ abstract contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
}
// Prevent enabling ragequit if distributions can be created without a vote.
- if (!_getSharedProposalStorage().opts.distributionsRequireVote)
+ if (_getSharedProposalStorage().opts.distributionsFlag == 0)
revert CannotEnableRageQuitIfNotDistributionsRequireVoteError();
uint40 oldRageQuitTimestamp = rageQuitTimestamp;
diff --git a/contracts/proposals/ProposalExecutionEngine.sol b/contracts/proposals/ProposalExecutionEngine.sol
index 5961339..212b26c 100644
--- a/contracts/proposals/ProposalExecutionEngine.sol
+++ b/contracts/proposals/ProposalExecutionEngine.sol
@@ -263,7 +263,7 @@ contract ProposalExecutionEngine is
_getSharedProposalStorage().opts.allowArbCallsToSpendPartyEth
);
} else if (pt == ProposalType.Distribute) {
- if (!_getSharedProposalStorage().opts.distributionsRequireVote) {
+ if (_getSharedProposalStorage().opts.distributionsFlag != 1) {
revert ProposalDisabled(pt);
}
diff --git a/contracts/proposals/ProposalStorage.sol b/contracts/proposals/ProposalStorage.sol
index 5a86652..2496595 100644
--- a/contracts/proposals/ProposalStorage.sol
+++ b/contracts/proposals/ProposalStorage.sol
@@ -33,7 +33,8 @@ abstract contract ProposalStorage {
// Whether operators can be used.
bool allowOperators;
// Whether distributions require a vote or can be executed by any active member.
- bool distributionsRequireVote;
+ // 0 = distributions don't require vote, 1 = distributions require vote, 2 = distributions disabled
+ uint8 distributionsFlag;
}
uint256 internal constant PROPOSAL_FLAG_UNANIMOUS = 0x1;Mitigation Review:
The recommendation has been implemented with a DistributionsConfig enum instead of a "raw" uint8 type:
enum DistributionsConfig {
AllowedWithoutVote,
AllowedWithVote,
NotAllowed
}The checks in the Party.distribute() function are implemented in a simpler way than in the recommendation but still semantically equivalent.
And the first check in the recommendation was in fact wrong but it has been corrected in this version of the report, while the fix implemented by the client is correct.
Description:
Assuming the BondingCurveAuthority has all fees set to zero, an attacker can flash-mint party cards and only
pay gas fees.
Thereby he can repeatedly update the lastTotalVotingPowerChangeTimestamp and cause a DoS in PartyGovernance.accept().
As a result proposals cannot be proposed or voted for.
Impact:
The attack must be renewed in every block.
Also, the BondingCurveAuthority fees will be greater than zero according to communication with the client.
Still, this attack could be viable in certain edge cases if the incentives for an attacker are sufficiently large.
Currently, there exists another problem which is that by providing an empty tokenIds array to BondingCurveAuthority.sellPartyCards(), the lastTotalVotingPowerChangeTimestamp can be updated without paying any protocol fees even if they are non-zero percentages.
Recommendation:
This issue has already been discussed in a previous audit (Link):
Caveat: Depending on the authority contracts in use, this might allow for a DOS attack in which an attacker continuously changes the totalVotingPower such as to prevent proposals from being proposed or votes from being cast.
The ideal solution would be to snapshot the totalVotingPower.
The long-term solution remains to snapshot totalVotingPower.
For now, as discussed with the client, the finding is acknowledged since snapshotting totalVotingPower is not feasible.
A partial fix is to revert in the BondingCurveAuthority.sellPartyCards() function if the amount is zero to ensure that the attacker has to pay the protocol fees:
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..1282957 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -301,6 +301,10 @@ contract BondingCurveAuthority {
}
uint80 amount = uint80(tokenIds.length);
+ if (amount == 0) {
+ revert InvalidMessageValue();
+ }
+
uint256 bondingCurvePrice = _getBondingCurvePrice(
partyInfo.supply - amount,
amount,Mitigation Review:
The recommended partial fix has been implemented.
Description:
The BondingCurveAuthority can only work as intended when no other authority increases or decreases the totalVotingPower of a party or changes the voting power of individual party cards.
There are multiple ways how a modification in the voting power bricks the BondingCurveAuthority. One of them is possible because the BondingCurveAuthority assumes that all party cards have the same voting power, as determined by the PARTY_CARD_VOTING_POWER constant. If the voting power of party cards is modified, it's possible to sell a party card with less than PARTY_CARD_VOTING_POWER voting power for the same price that it's possible to sell a party card with PARTY_CARD_VOTING_POWER or more voting power. This obviously bricks the pricing mechanism.
Impact:
The enableAddAuthorityProposal flag is set upon construction of the party and cannot be changed.
Therefore it is a mistake on the user's end if he participates in a party with such a potentially dangerous setting enabled.
Recommendation:
Validate the enableAddAuthorityProposal flag and revert if it is set to true.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..eb26e94 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -20,6 +20,7 @@ contract BondingCurveAuthority {
error ExecutionDelayTooShort();
error EthTransferFailed();
error ExcessSlippage();
+ error InvalidEnableAddAuthorityProposal();
event TreasuryFeeUpdated(uint16 previousTreasuryFee, uint16 newTreasuryFee);
event PartyDaoFeeUpdated(uint16 previousPartyDaoFee, uint16 newPartyDaoFee);
@@ -214,6 +215,10 @@ contract BondingCurveAuthority {
if (partyOpts.governance.executionDelay < MIN_EXECUTION_DELAY) {
revert ExecutionDelayTooShort();
}
+
+ if (partyOpts.proposalEngine.enableAddAuthorityProposal) {
+ revert InvalidEnableAddAuthorityProposal();
+ }
}The fix ensures that a party created with the BondigCurveAuthority cannot accidentally or on purpose add another authority to brick the BondingCurveAuthority.
Mitigation Review:
The recommendation has been implemented and the enableAddAuthorityProposal flag is checked upon creating a Party.
Now, a Party created with the BondingCurveAuthority contract can only have this contract set as an authority.
Description:
The creator of a party is an untrusted role and so the success of the creatorFee transfer is not checked in case the creator reverts.
The problem is that the user still has to pay for the creatorFee even if the transfer fails.
Impact:
Since it's just a safety mechanism against DoS to not check the success of the creatorFee transfer and in almost all cases the transfer should succeed, the issue is only of "Low" severity.
Recommendation:
In BondingCurveAuthority.buyPartyCards(), the totalCost should be reduced by creatorFee if the creatorFee transfer fails.
And in BondingCurveAuthority.sellPartyCards(), the sellerProceeds should be increased by creatorFee if the creatorFee transfer fails.
In both cases, the checks for msg.value < totalCost and sellerProceeds < minProceeds respectively must take place after the creatorFee transfer.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..f403470 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -246,10 +246,6 @@ contract BondingCurveAuthority {
BPS;
uint256 totalCost = bondingCurvePrice + partyDaoFee + treasuryFee + creatorFee;
- if (amount == 0 || msg.value < totalCost) {
- revert InvalidMessageValue();
- }
-
partyInfos[party].supply = partyInfo.supply + amount;
(bool success, ) = address(party).call{ value: treasuryFee }("");
@@ -259,8 +255,16 @@ contract BondingCurveAuthority {
if (creatorFee != 0) {
// Creator fee payment can fail
- partyInfo.creator.call{ value: creatorFee }("");
+ (success, ) = partyInfo.creator.call{ value: creatorFee }("");
+ if (!success) {
+ totalCost -= creatorFee;
+ }
+ }
+
+ if (amount == 0 || msg.value < totalCost) {
+ revert InvalidMessageValue();
}
+
partyDaoFeeClaimable += partyDaoFee.safeCastUint256ToUint96();
party.increaseTotalVotingPower(PARTY_CARD_VOTING_POWER * amount);
@@ -318,9 +322,6 @@ contract BondingCurveAuthority {
treasuryFee -
creatorFee -
amount;
- if (sellerProceeds < minProceeds) {
- revert ExcessSlippage();
- }
partyInfos[party].supply = partyInfo.supply - amount;
@@ -344,7 +345,14 @@ contract BondingCurveAuthority {
if (creatorFee != 0) {
// Creator fee payment can fail
- partyInfo.creator.call{ value: creatorFee }("");
+ (success, ) = partyInfo.creator.call{ value: creatorFee }("");
+ if (!success) {
+ sellerProceeds += creatorFee;
+ }
+ }
+
+ if (sellerProceeds < minProceeds) {
+ revert ExcessSlippage();
}
(success, ) = msg.sender.call{ value: sellerProceeds }("");Mitigation Review:
The creatorFee is now refunded if the ETH transfer fails.
What has not been implemented is to move the sellerProceeds < minProceeds check in sellPartyCards() after the optional refund of the creatorFee Link.
If the refund of creatorFee should be taken into account, the check should only be performed after the attempted ETH transfer as shown in the recommendation.
Description:
When a new party is created, its governance parameters are checked in the _validateGovernanceOpts() function.
There are two problems. First, executionDelay only needs to be greater or equal to one. This is unsafe since an executionDelay that is too small doesn't allow the hosts to veto unsafe / malicious proposals.
In reality, there exist strong incentives for an attacker to mint party cards for a short duration of time (1 block is enough) to then try to takeover the party with a malicious proposal.
The second problem is that it is not checked that the hosts array has a length greater zero, i.e., whether hosts are assigned.
If no hosts are assigned, proposals cannot be vetoed.
Impact:
The missing sanity checks allow for unsafe deployments of parties.
Participating in such an unsafe party is a user error. Still, it is better to at least provide better sanity checks for new party deployments.
Recommendation:
It should be checked that executionDelay has a reasonable value. Such a reasonable value could be 1 hour.
Also, it should be checked that the number of hosts is greater zero.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..b26726c 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -20,6 +20,7 @@ contract BondingCurveAuthority {
error ExecutionDelayTooShort();
error EthTransferFailed();
error ExcessSlippage();
+ error InvalidHosts();
event TreasuryFeeUpdated(uint16 previousTreasuryFee, uint16 newTreasuryFee);
event PartyDaoFeeUpdated(uint16 previousPartyDaoFee, uint16 newPartyDaoFee);
@@ -63,7 +64,7 @@ contract BondingCurveAuthority {
uint16 private constant MAX_TREASURY_FEE = 1000; // 10%
uint16 private constant MAX_PARTY_DAO_FEE = 250; // 2.5%
/// @notice The minimum execution delay for party governance
- uint40 private constant MIN_EXECUTION_DELAY = 1 seconds;
+ uint40 private constant MIN_EXECUTION_DELAY = 1 hours;
/// @notice Struct containing options for creating a party
struct BondingCurvePartyOptions {
@@ -207,6 +208,9 @@ contract BondingCurveAuthority {
if (partyOpts.governance.totalVotingPower != 0) {
revert InvalidTotalVotingPower();
}
+ if (partyOpts.governance.hosts.length == 0) {
+ revert InvalidHosts();
+ }
// Note: while the `executionDelay` is not enforced to be over 1 second,
// it is strongly recommended for it to be a long period
// (greater than 1 day). This prevents an attacker from buying cards,Mitigation Review:
Neither of the suggestions has been implemented. The finding is acknowledged. It depends on the Party creator to safely set the Party's parameters.
The totalPrice is not msg.value. Instead it is totalCost.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..9191992 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -273,7 +273,7 @@ contract BondingCurveAuthority {
party,
msg.sender,
tokenIds,
- msg.value,
+ totalCost,
partyDaoFee,
treasuryFee,
creatorFeeMitigation Review:
The recommendation has been implemented.
Due to rounding it is possible that for each party card the buy price is 1 wei less than the seller proceeds.
To mitigate any scenarios where the funds in the BondingCurveAuthority aren't sufficient to pay the seller, the rounding error is accounted for and amount is subtracted from bondingCurvePrice in the sellPartyCards() function.
It would be better to instead add amount to totalPrice in buyPartyCards() since the subtraction of amount can in theory cause an underflow.
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..eca3e86 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -244,7 +244,7 @@ contract BondingCurveAuthority {
uint256 treasuryFee = (bondingCurvePrice * treasuryFeeBps) / BPS;
uint256 creatorFee = (bondingCurvePrice * (partyInfo.creatorFeeOn ? creatorFeeBps : 0)) /
BPS;
- uint256 totalCost = bondingCurvePrice + partyDaoFee + treasuryFee + creatorFee;
+ uint256 totalCost = bondingCurvePrice + partyDaoFee + treasuryFee + creatorFee + amount;
if (amount == 0 || msg.value < totalCost) {
revert InvalidMessageValue();
@@ -316,8 +316,7 @@ contract BondingCurveAuthority {
uint256 sellerProceeds = bondingCurvePrice -
partyDaoFee -
treasuryFee -
- creatorFee -
- amount;
+ creatorFee;
if (sellerProceeds < minProceeds) {
revert ExcessSlippage();
}
@@ -385,8 +384,7 @@ contract BondingCurveAuthority {
partyDaoFeeBps -
treasuryFeeBps -
(partyInfo.creatorFeeOn ? creatorFeeBps : 0))) /
- BPS -
- amount;
+ BPS;
}
/**
@@ -426,7 +424,7 @@ contract BondingCurveAuthority {
uint256 bondingCurvePrice = _getBondingCurvePrice(supply, amount, a, b);
return
(bondingCurvePrice *
- (BPS + partyDaoFeeBps + treasuryFeeBps + (creatorFeeOn ? creatorFeeBps : 0))) / BPS;
+ (BPS + partyDaoFeeBps + treasuryFeeBps + (creatorFeeOn ? creatorFeeBps : 0))) / BPS + amount;
}
/**Mitigation Review:
The recommendation has been implemented.
9. getPriceToBuy() and getSaleProceeds() view functions provide less favorable result than actual prices 
Due to rounding the getPriceToBuy() and getSaleProceeds() view functions can provide results that are slightly off from the actual prices that a user would get by calling buyPartyCards() or sellPartyCards().
In all cases is the result of the view functions less favorable than the actual price.
Still, for some use cases it can be beneficial to provide exact results. It is therefore recommended to use exactly the same calculations in the view functions that are also done in buyPartyCards() and sellPartyCards().
diff --git a/contracts/authorities/BondingCurveAuthority.sol b/contracts/authorities/BondingCurveAuthority.sol
index 7a7eb83..a55dd21 100644
--- a/contracts/authorities/BondingCurveAuthority.sol
+++ b/contracts/authorities/BondingCurveAuthority.sol
@@ -378,15 +378,18 @@ contract BondingCurveAuthority {
partyInfo.a,
partyInfo.b
);
+ uint256 partyDaoFee = (bondingCurvePrice * partyDaoFeeBps) / BPS;
+ uint256 treasuryFee = (bondingCurvePrice * treasuryFeeBps) / BPS;
+ uint256 creatorFee = (bondingCurvePrice * (partyInfo.creatorFeeOn ? creatorFeeBps : 0)) /
+ BPS;
+
// Note: 1 is subtracted for each NFT to account for rounding errors
- return
- (bondingCurvePrice *
- (BPS -
- partyDaoFeeBps -
- treasuryFeeBps -
- (partyInfo.creatorFeeOn ? creatorFeeBps : 0))) /
- BPS -
+ uint256 sellerProceeds = bondingCurvePrice -
+ partyDaoFee -
+ treasuryFee -
+ creatorFee -
amount;
+ return sellerProceeds;
}
/**
@@ -424,9 +427,12 @@ contract BondingCurveAuthority {
bool creatorFeeOn
) public view returns (uint256) {
uint256 bondingCurvePrice = _getBondingCurvePrice(supply, amount, a, b);
- return
- (bondingCurvePrice *
- (BPS + partyDaoFeeBps + treasuryFeeBps + (creatorFeeOn ? creatorFeeBps : 0))) / BPS;
+ uint256 partyDaoFee = (bondingCurvePrice * partyDaoFeeBps) / BPS;
+ uint256 treasuryFee = (bondingCurvePrice * treasuryFeeBps) / BPS;
+ uint256 creatorFee = (bondingCurvePrice * (creatorFeeOn ? creatorFeeBps : 0)) /
+ BPS;
+ uint256 totalCost = bondingCurvePrice + partyDaoFee + treasuryFee + creatorFee;
+ return totalCost;
}Mitigation Review:
The recommendation has been implemented. The calculations in getPriceToBuy() and getSaleProceeds() match the calculations in buyPartyCards() and sellPartyCards().