Description:
Events should be the last occurence of each function to signal the success of the user action. TokenDivider emits events before an action is completed. For example, in the function TokenDivider::sellErc20() the event is emited before the transaction. If TokenDivider::sellErc20() fails at the token transaction, the function will revert. But, even though the function reverts, the event is already pushed giving the impression that the TokenDivider::sellErc20() succeded.
@> emit OrderPublished(amount, msg.sender, nftPegged);
IERC20(tokenInfo.erc20Address).transferFrom(msg.sender, address(this), amount);
The events should be placed last in the following functions:
TokenDivider::divideNft
TokenDivider::claimNft
TokenDivider::sellErc20
TokenDivider::buyOrder
Impact:
This issue happens every time a function reverts after the event is emited. Very often.
Proof of Concept:
function testMySell() public {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
}
The function sellErc20() will revert after the event is emited (because the user did not aprove the transaction), as you can see in the logs.
├─ [117709] TokenDivider::sellErc20(ERC721Mock: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000 [1e18], 2000000000000000000 [2e18])
@> │ ├─ emit OrderPublished(amount: 2000000000000000000 [2e18], seller: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], nftPegged: ERC721Mock: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ ├─ [2966] ERC20ToGenerateNftFraccion::transferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], TokenDivider: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 2000000000000000000 [2e18])
│ │ └─ ← [Revert] ERC20InsufficientAllowance(0x90193C961A926261B756D1E5bb255e67ff9498A1, 0, 2000000000000000000 [2e18])
│ └─ ← [Revert] ERC20InsufficientAllowance(0x90193C961A926261B756D1E5bb255e67ff9498A1, 0, 2000000000000000000 [2e18])
└─ ← [Revert] ERC20InsufficientAllowance(0x90193C961A926261B756D1E5bb255e67ff9498A1, 0, 2000000000000000000 [2e18])
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 8.30ms (1.96ms CPU time)
Recommended Mitigation:
For each event, change the order in the function. For example, at TokenDivider::sellErc20():
function sellErc20(address nftPegged, uint256 price,uint256 amount) external {
if(nftPegged == address(0)) {
revert TokenDivider__NftAddressIsZero();
}
if( amount == 0) {
revert TokenDivider__AmountCantBeZero();
}
ERC20Info memory tokenInfo = nftToErc20Info[nftPegged];
if(balances[msg.sender][tokenInfo.erc20Address] < amount) {
revert TokenDivider__InsuficientBalance();
}
balances[msg.sender][tokenInfo.erc20Address] -= amount;
s_userToSellOrders[msg.sender].push(
SellOrder({
seller: msg.sender,
erc20Address: tokenInfo.erc20Address,
price: price,
amount: amount
})
);
- emit OrderPublished(amount,msg.sender, nftPegged);
IERC20(tokenInfo.erc20Address).transferFrom(msg.sender,address(this), amount);
+ emit OrderPublished(amount,msg.sender, nftPegged);
}
The events need to be changed in the following functions:
TokenDivider::divideNft
TokenDivider::claimNft
TokenDivider::sellErc20
TokenDivider::buyOrder
