Description:
The TokenDivider contract keeps track of user's balances for every ERC20 token generated when dividing an NFT through the balances mapping. This mapping is not updated correctly when the user calls the ERC20ToGenerateNftFraccion::transfer function (inherited from the ERC20 standard contract).
Impact:
This vulnerability causes a potential misalignment between the balances stored in the mapping and the actual balances possessed by users. This way, we cannot trust the balances mapping to perform operations in the smart contract.
Tools Used:
Manual review
Proof of Concept:
Add the following test to TokenDividerTest.t.sol. USER transfers all the ERC20 tokens generated by the division of their NFT to USER2. In the assert statement, we assess that both USER and USER2 balances remain unchanged by the operation, whereas the balances retrived via the balanceOf function of the ERC20Mock contract unveils a different situation, where the balances are, in fact, the opposite of what showed by the TokenDivider::balances mapping.
function testTransferWithERC20StandardFunction() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
vm.prank(USER);
erc20Mock.transfer(USER2, AMOUNT);
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), AMOUNT);
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), 0);
assertEq(erc20Mock.balanceOf(USER), 0);
assertEq(erc20Mock.balanceOf(USER2), AMOUNT);
}
Recommended Mitigation:
Create two external functions in TokenDivider which will be responsible for increasing and decreasing the TokenDivider::balances mapping.
ATTENTION: these two functions should have access control. It is recommended to keep track of the ERC20 contracts addresses and grant them exclusive permission to call the following two functions.
+ function decreaseBalance(address user, address erc20Address, uint256 amount) external {
+ balances[user][erc20Address] -= amount;
+ }
+ function increaseBalance(address user, address erc20Address, uint256 amount) external {
+ balances[user][erc20Address] += amount;
+ }
Override the ERC20ToGenerateNftFraccion::transfer function in order to update the TokenDivider::balances mapping whenever a transfer occurs. Add a check at the top to make sure that the mapping is not updated when first trasferring ERC20 tokens from the TokenDivider contract to the user that has deposited their NFT.
import {ERC20Burnable} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+ import {TokenDivider} from "../TokenDivider.sol";
contract ERC20ToGenerateNftFraccion is ERC20, ERC20Burnable {
+ TokenDivider tokenDivider;
- constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) {
-
- }
+ constructor(string memory _name, string memory _symbol, address _tokenDivider) ERC20(_name, _symbol) {
+ tokenDivider = TokenDivider(_tokenDivider);
+ }
function mint(address _to, uint256 _amount) public {
_mint(_to, _amount);
}
+ function transfer(address to, uint256 value) public override returns (bool) {
+ address owner = _msgSender();
+ if(owner != address(tokenDivider)) {
+ tokenDivider.decreaseBalance(owner, address(this), value);
+ }
+ tokenDivider.increaseBalance(to, address(this), value);
+ _transfer(owner, to, value);
+ return true;
+ }
}
In addition, remove the update logic from TokenDivider::divideNft, to avoid conflicts and double update of TokenDivider::balances mapping.
function divideNft(address nftAddress, uint256 tokenId, uint256 amount) onlyNftOwner(nftAddress, tokenId) onlyNftOwner(nftAddress ,tokenId) external {
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
if(amount == 0) { revert TokenDivider__AmountCantBeZero(); }
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol())));
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
- balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
emit NftDivided(nftAddress, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if(!transferSuccess) {
revert TokenDivider__TransferFailed();
}
}
Add the following test to TokenDividerTest.t.sol to make sure that the TokenDivider::balances mapping updates correctly.
function testTransferWithERC20StandardFunctionUpdatesMapping() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
vm.prank(USER);
erc20Mock.transfer(USER2, AMOUNT);
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), AMOUNT);
assertEq(erc20Mock.balanceOf(USER), 0);
assertEq(erc20Mock.balanceOf(USER2), AMOUNT);
}
