search

aavegotchi / aavegotchi-realm-diamond

23 stars 9 forks source link

[Audit Report] [N6] [Suggestion] Contract transfer reminder #20

Closed orionstardust closed 2 years ago

orionstardust commented 2 years ago

Description

In the AlchemicaFacet contract, the exitAlchemica function transfer the AlchemicaToken to the gotchi through the transferFrom function and the Alchemica Token needs to be approved to this contract.

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/RealmDiamond/facets/AlchemicaFacet.sol#L345

Solution

It is recommended to use the transfer function directly for transfers in this contract.

cinnabarhorse commented 2 years ago

We created the special ExitAlchemica function to be a permissioned function only for the GameManager, and also to emit the ExitAlchemica event so that the game frontend can index the event onchain. It's not possible for normal players to transfer alchemica from the contract to gotchis/owners.

mudgen commented 2 years ago

@cinnabarhorse To clarify things, the exitAlchemica function has this line of code in it:

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/RealmDiamond/facets/AlchemicaFacet.sol#L361

That line of code could be changed to this which is a little better: alchemica.transfer(alchemicaRecipient(_gotchiId), _alchemica[i]);

The reason this can be a little better is because transferFrom might require an approval from the sender, and transfer does not.