Vulnerability Details
The current protocol logic set for BuyerAgent::purchase() is, it loops through listed assets for the round, pushes each asset into inventory and purchases it. BuyerAgent also has a amountPerRoundthat ensures that spendings per round don't cross the specified threshold. During the traversal through the assets, a spendings[round] is incremented by asset price to check whether the spendings so far for the ongoing round exceeds the amountPerRound or not. If yes, the whole process reverts!
if (spendings[round] > amountPerRound) {
revert BuyLimitExceeded(spendings[round], amountPerRound);
}
A revert in BuyerAgent::purchase() implies the following,
Sellers pay royalty to list the assets. A revert means seller loses royalty and doesn't get paid for the the NFT they want to sell.
Basically, buyer & swan owner gain royalty & platformFee from the seller and since purchase would revert, seller won't get their worth of the asset they listed.
Relisting requires paying the royality again so it's not like they can try again. The same thing could happen again!
Gas during execution of listing is wasted since their asset cannot be purchased for the current round.
Owner/operator cannot change the amountPerRound during the buy phase so nothing can be done about this at this stage.
An attacker can take advantage of this to list their asset with enough price so spendings[round] exceeds the amountPerRound resulting in revert in purchase(), leading to no earnings for the sellers. Currently, there are no mechanisms in Swan::list to prevent such a thing from happening. The Swan::list just keeps adding assets as long as maxAssetCount isn't crossed, without looking at the asset price crossing the amountPerRound, directly impacting the BuyerAgent::purchase.
Impact
Sellers pay royalty to get their asset listed, but since their assets would remain unsold, they won't receive tokens. So the token they paid as royalty is essentially lost. Relisting tokens requires paying royalty again, discouraging sellers from using platform again, leading to a lower overall platform activity.
Proof-Of-Code
Add the approveAndlistAssets() function in contracts/test/helpers/index.ts and the test in Swan.test.ts.
Details:
4 sellers (including attacker) list their asset for a buyer to purchase.
The amountPerRound is set to 50.
Sellers list their assets. Attacker maliciously lists the asset with higher amount(40) to exceed amountPerRound.
The BuyerAgent::purchase() is triggered & it reverts for the ongoing round.
export async function approveAndlistAssets(
swan: Swan,
buyerAgent: BuyerAgent,
creators: [HardhatEthersSigner, bigint][],
name: string,
symbol: string,
description: string,
round: bigint,
token:any
) {
for (const [creator, val] of creators) {
await token.connect(creator).approve(await swan.getAddress(), val);
await swan.connect(creator).list(name, symbol, description, val, await buyerAgent.getAddress());
}
const assets = await swan.getListedAssets(await buyerAgent.getAddress(), round);
expect(assets.length).to.equal(creators.length);
}
it("purchase reverts when amountPerRound is exceeded", async() => {
const supply = parseEther("1000");
token = await deployTokenFixture(dria, supply);
expect(await token.balanceOf(dria.address)).to.eq(supply);
const MARKET_PARAMETERS2 = {
withdrawInterval: minutes(30),
sellInterval: minutes(60),
buyInterval: minutes(10),
platformFee: 10n,
maxAssetCount: 5n,
timestamp: 0n,
} satisfies SwanMarketParametersStruct;
const currentTime = (await ethers.provider.getBlock("latest").then((block) => block?.timestamp)) as bigint;
MARKET_PARAMETERS2.timestamp = currentTime;
({ swan, registry, coordinator } = await deploySwanFixture(
dria,
token,
STAKES,
FEES,
MARKET_PARAMETERS2,
ORACLE_PARAMETERS
));
const amountPerRound = ethers.parseEther("50")
const buyerAgentParams = [
{
name: "BuyerAgent#1",
description: "Description of BuyerAgent 1",
royaltyFee: ROYALTY_FEE,
amountPerRound: amountPerRound,
owner: buyer,
},
];
[buyerAgent, buyerAgentToFail] = await createBuyers(swan, buyerAgentParams);
await transferTokens(token, [
[buyer.address, parseEther("100")],
[seller1.address, parseEther("20")],
[seller2.address, parseEther("30")],
[seller3.address, parseEther("30")],
[attacker.address, parseEther("50")],
[generator.address, STAKE_AMOUNT],
[validator.address, STAKE_AMOUNT],
])
await token.connect(buyer).transfer(await buyerAgent.getAddress(), parseEther("100"));
await registerOracles(token, registry, [generator], [validator], {
generatorStakeAmount: STAKE_AMOUNT,
validatorStakeAmount: STAKE_AMOUNT,
})
await approveAndlistAssets(
swan,
buyerAgent,
[
[seller1, parseEther("5")],
[attacker, parseEther("40")],
[seller2, parseEther("11")],
[seller3, parseEther("7")],
],
NAME,
SYMBOL,
DESC,
0n, token
)
const assetsToBuy = await swan.getListedAssets(
await buyerAgent.getAddress(),
0n
);
await time.increase(MARKET_PARAMETERS2.sellInterval);
await buyerAgent.connect(buyer).oraclePurchaseRequest("0x", "0x");
const output = new AbiCoder().encode(["address[]"], [assetsToBuy]);
await safeRespond(coordinator, generator, output, "0x", 1n, 0n);
await safeValidate(coordinator, validator, [parseEther("1")], "0x", 1n, 0n);
await expect(buyerAgent.connect(buyer).purchase()).to.be.revertedWithCustomError(buyerAgent, "BuyLimitExceeded");
})
Tools Used
Manual Review & Hardhat testing.
Recommendations
Consider one of the following,
Royalty should be paid once the purchase of the asset is successful, not when it is listed.
Loop should continue instead, since it's possible that first asset's price exceeds the amountPerRound, but adding price of the rest
doesn't push spendings[round] above amountPerRound.
function purchase() external onlyAuthorized {
// check that we are in the Buy phase, and return round
// review where are these phases changed?
(uint256 round,) = _checkRoundPhase(Phase.Buy);
...
...
for (uint256 i = 0; i < assets.length; i++) {
address asset = assets[i];
uint256 price = swan.getListingPrice(asset);
spendings[round] += price;
if (spendings[round] > amountPerRound) {
- revert BuyLimitExceeded(spendings[round], amountPerRound);
+ spendings[round] -= price;
+ continue;
}
// add to inventory
// @self-insights asset bought is stored against the round in which it was bought.
inventory[round].push(asset);
// make the actual purchase
swan.purchase(asset);
}
}
3.The if (spendings[round] > amountPerRound) should be checked for in Swan::list() instead of in BuyerAgent::purchase so it reverts there and then
if the current listings for the round exceeds amountPerRound.
+ error BuyLimitExceeded(uint256 have, uint256 want);
function list(string calldata _name, string calldata _symbol, bytes calldata _desc, uint256 _price, address _buyer)
external
{
BuyerAgent buyer = BuyerAgent(_buyer);
(uint256 round, BuyerAgent.Phase phase,) = buyer.getRoundPhase();
if (phase != BuyerAgent.Phase.Sell) {
revert BuyerAgent.InvalidPhase(phase, BuyerAgent.Phase.Sell);
}
+ if (buyer.spendings[round] + _price > buyer.amountPerRound()) {
+ revert BuyLimitExceeded(spendings[round] + _price, amountPerRound);
+ }
...
...
}
4.The BuyerAgent::setAmountPerRound() should be allowed if BuyerAgent::purchase breaks, flipping a boolean. This should only be in case
of a revert.
5.No royalty should be paid on relisting tokens. Since royalty has already been paid on them.
