Changes after sale enable theft of funds#
High Risk
In Create.sol there is updateCanvas
function that is used to update a canvas.
You can update saleStart
and saleEnd
as long as the sale has not started, or the sale ended, and it is not a refundable dutch.
But changes to saleStart
and saleEnd
after sale ends enables other changes to also be made. This enables stealing of funds.
Examples#
Example 1#
- User A creates a canvas and sets it up for a sale with
saleStart
set to current date, 7am andsaleEnd
current date, 8am. Price is 10 ETH - He buys an NFT that is part of the canvas for 10 ETH. He receives the 10 ETH back because he is buying from himself.
- It is now 9am and the sale expired.
- He updates canvas and sets new
saleStart
to 10am andsaleEnd
to 11am. - Now the
saleStarted
check is false. So user A can again execute a canvas update and set it as refundable dutch with end price of 1 ETH. - He buys another NFT through the refundable dutch for 1 ETH.
- He executes the
claimDutchRefund
for the first NFT (that he bought on a regular sale actually) and receives 9 ETH. - He executes the
payoutAction
and receivescanvas.totalQuantity * ds.canvasSystem[canvasId].dutchEndPrice
=2 * 1 ETH
= 2 ETH. - He waits a year to go by and executes the
claimExpiredRefund
. - The amount that he can claim is calculated
ds.canvasSystem[canvasId].revenue - (canvas.totalQuantity * ds.canvasSystem[canvasId].dutchEndPrice)) - (ds.canvasSystem[canvasId].refundSum) / 2
=11 ETH - (2 * 1 ETH) - 9 ETH / 2
=4.5 ETH
Example 2#
- User A creates a canvas and sets it up for a sale with
saleStart
set to current date, 7am andsaleEnd
current date, 8am. Price is 1 ETH - He buys an NFT that is part of the canvas for 1 ETH. He receives the 1 ETH back because he is buying from himself.
- It is now 9am and the sale expired.
- He updates canvas and sets new
saleStart
to 10am andsaleEnd
to 11am. - Now the
saleStarted
check is false. So user A can again execute a canvas update and set it as refundable dutch with end price of 10 ETH. - He buys another NFT through the refundable dutch for 10 ETH.
- He executes the
payoutAction
and receivescanvas.totalQuantity * ds.canvasSystem[canvasId].dutchEndPrice
=2 * 10 ETH
= 20 ETH.
In first example user started with 10 ETH, bought his own NFT which means he received the 10 ETH back. Bought his own NFT again for 1 ETH. Then he got paid 2 ETH from the contract. And in the end he claimed 4.5 ETH and ended up with 15.5 ETH.
In second example user started with 10 ETH, bought his own NFT for 1 ETH which he received back. Then he bought his own NFT for 10 ETH and got paid from the contract 20 ETH. So he ended up with 20 ETH. This is not taking into account the protocol fee, which can be 0 - 30 %.
Recommendation#
Either prevent changes to saleStart
(and consequently type of sale and price) after there has already been buying/selling done on that canvas, or prevent a canvas that was previously on a sale to be marked for refundable dutch.
The first option would be preferred, because changing saleStart
has many side effects and that opens up more possibilities for something to be vulnerable.