[WP-H9] Improper access control may result in users' fund loss

[code423n4]

Lines of code

https://github.com/maple-labs/mpl-migration/blob/a99549d96ed12cd4589a02bccf70747dbaebeb5b/contracts/Migrator.sol#L24-L28

Vulnerability details

https://github.com/maple-labs/mpl-migration/blob/a99549d96ed12cd4589a02bccf70747dbaebeb5b/contracts/Migrator.sol#L24-L28

function migrate(address owner, uint256 amount_) public {
    require(amount_ > uint256(0),                                              "M:M:ZERO_AMOUNT");
    require(ERC20Helper.transferFrom(oldToken, owner, address(this), amount_), "M:M:TRANSFER_FROM_FAILED");
    require(ERC20Helper.transfer(newToken, owner, amount_),                    "M:M:TRANSFER_FAILED");
}

The migrate() allows anyone to use other's allowance to migrate oldToken to newToken.

newToken is not necessarily valued higher or equal to oldToken, therefore, migrating oldToken to newToken against users' will can result in users' fund loss.

PoC

Given:

• Alice has 10,000 oldToken

1. Alice approved type(uint256).max of oldToken to the contract;
2. Alice called migrate() and migrated 1,000 oldToken to newToken;
3. for some reason, the market price of oldToken is now greater than newToken, Alice decided not to migrate the rest old tokens;
4. the attacker called migrate() and migrated Alice's 9,000 oldToken to newToken.

Recommendation

Change to:

function migrate(uint256 amount_) external {
    _migrate(msg.sender, amount_);
}

function _migrate(address owner, uint256 amount_) internal {
    require(amount_ > uint256(0),                                              "M:M:ZERO_AMOUNT");
    require(ERC20Helper.transferFrom(oldToken, owner, address(this), amount_), "M:M:TRANSFER_FROM_FAILED");
    require(ERC20Helper.transfer(newToken, owner, amount_),                    "M:M:TRANSFER_FAILED");
}

[lucas-manuel]

This is intentional.

If Alice approves type(uint256).max to the migrator contract, she is allowing that contract to migrate her tokens.

Worst case the tokens get migrated and sent to her address. The situation where a user would not want to migrate is not feasible in our opinion. This is akin to LEND => AAVE.

[dmvt]

Agree with sponsor. This is a feature. Alice could always revoke the approval or send oldToken to a different account.