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
saleStartset to current date, 7am andsaleEndcurrent 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
saleStartto 10am andsaleEndto 11am. - Now the
saleStartedcheck 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
claimDutchRefundfor the first NFT (that he bought on a regular sale actually) and receives 9 ETH. - He executes the
payoutActionand 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
saleStartset to current date, 7am andsaleEndcurrent 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
saleStartto 10am andsaleEndto 11am. - Now the
saleStartedcheck 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
payoutActionand 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.