Description
Code
function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
previousOwner = super._update(to, tokenId, auth);
if (to != address(0) && previousOwner != address(0)) {
router.afterUpdate(previousOwner, to, tokenId);
}
}
As you can see it’s interaction first and then checks & effects which violates the CEI principle
PoC
Consider the following 2 actors. Initially Actor 1 (bob) adds liquidity then later transfers the Lp NFT to Charlie. Charlie is eager to remove liquidity as it now owns the NFT, but will fail. This is because the LPNFT::_update() function does not follow the Checks Effects Interaction pattern
Impact: There is an external hook when Charlie is the owner of the token, yet cannot access liquidity funds because they are still associated to the previous owner.
Actor 1
Actor 2
contract Charlie is IERC721Receiver {
UpliftOnlyExample uplift;
uint256 bptAmount;
uint256[] minAmountsOut;
address pool;
constructor(UpliftOnlyExample _uplift, uint256 _bptAmountIn, uint256[] memory _minAmountsOut, address _pool) {
uplift = _uplift;
bptAmount = _bptAmountIn;
minAmountsOut = _minAmountsOut;
pool = _pool;
}
function onERC721Received(
address _operator,
address _from,
uint256 _tokenId,
bytes calldata _data
) external override returns (bytes4) {
On receiving the tokenID from bob, charlie would try to remove liquidity
but will fail
*/
uplift.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
return this.onERC721Received.selector;
}
}
Test function
function testBurnedTokenReused() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
LPNFT lpNft = upliftOnlyRouter.lpNFT();
assert(lpNft.ownerOf(1) == bob);
Charlie charlie = new Charlie(upliftOnlyRouter, bptAmount, minAmountsOut, pool);
vm.expectRevert();
lpNft.safeTransferFrom(bob, address(charlie), 1);
}
Mitigation:
Change the order in which calls are made
function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
- previousOwner = super._update(to, tokenId, auth);
if (to != address(0) && previousOwner != address(0)) {
router.afterUpdate(previousOwner, to, tokenId);
}
+ previousOwner = super._update(to, tokenId, auth);
}
