TokeMigrationPool Manual Review Findings

TMP-01M: Unsanitized State Transition

Description:

The setEventSend function should only set the _eventSend value to true when the values of the destinations struct have been set.

Example:

Copy
255function setEventSend(bool _eventSendSet) external override onlyOwner {
256    _eventSend = _eventSendSet;
257
258    emit EventSendSet(_eventSendSet);
259}

Recommendation:

We advise such sanitization to be imposed via corresponding require checks as otherwise almost all functions will become inexecutable.

Alleviation:

The function can now only be executed when the destinations.destinationOnL2 value has been set.

TMP-02M: Deviation of Balance Layer 2 Relay

Description:

The TokeMigrationPool subtracts the value of the requestedWithdrawals of a user during the layer 2 relayed message in contrast to the EthPool and Pool implementations.

Example:

Copy
276function encodeAndSendData(bytes32 _eventSig, address _user) private onEventSend {
277    require(address(destinations.fxStateSender) != address(0), "ADDRESS_NOT_SET");
278    require(destinations.destinationOnL2 != address(0), "ADDRESS_NOT_SET");
279
280    uint256 userBalance = balanceOf(_user).sub(requestedWithdrawals[_user].amount);
281    bytes memory data = abi.encode(BalanceUpdateEvent({
282        eventSig: _eventSig,
283        account: _user, 
284        token: address(this), 
285        amount: userBalance
286    }));
287
288    destinations.fxStateSender.sendMessageToChild(destinations.destinationOnL2, data);
289}

Recommendation:

We advise this trait of the system to be re-evaluated and consistent behaviour to be introduced across the contract.

Alleviation:

The Tokemak team has stated that this is indeed desired behaviour and as such will not apply a remediation for it.

TMP-03M: Potentially Improper Overriddance of Withdrawal State

Description:

The withdrawAndMigrate function deletes the latest requestedWithdrawals entry regardless of whether the balanceOf the user equals the actual requested withdrawal.

Example:

Copy
123function withdrawAndMigrate() external whenNotPaused nonReentrant {
124    address stakingContract = 0x96F98Ed74639689C3A11daf38ef86E59F43417D3;
125
126    uint256 amount = balanceOf(msg.sender); // 1:1 allows this operation to work
127    require(amount > 0, "NOTHING_TO_MIGRATE");
128    
129    WithdrawalInfo memory withdrawal = requestedWithdrawals[msg.sender];
130    uint256 requestAmount = withdrawal.amount;
131    if (requestAmount > 0) {
132        withheldLiquidity = withheldLiquidity.sub(requestAmount);
133        delete requestedWithdrawals[msg.sender];
134    }
135
136    _approveStaking(amount, stakingContract);
137    _burn(msg.sender, amount);
138
139    IStaking(stakingContract).depositFor(msg.sender, amount, 0);
140
141    bytes32 eventSig = "Withdraw";
142    encodeAndSendData(eventSig, msg.sender);
143
144    emit Migrated(msg.sender, amount);
145}

Recommendation:

We advise some form of validation to be performed whereby the withdrawal.amount is validated to be less-than-or-equal-to (<=) or equal-to (==) the value of amount depending on the expected logical state.

Alleviation:

The Tokemak team has stated that the function is meant to transfer all assets and regardless of the amount specified the logical final state of requested withdrawals should be 0. As such, we consider this exhibit null.