Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

The transaction of the GMXVault#`constructor()` would be revert if `$UNI` token would be used as a tokenA or tokenB

0xmuxyz

Summary

According to the Revert on Large Approvals & Transfers, $UNI token can not be approved with the amount, which is larger than uint96 like this:

Some tokens (e.g. UNI, COMP) revert if the value passed to approve or transfer is larger than uint96.

However, within the GMXVault#constructor(), each Vault would be approved to spend the type(uint256).max amount of any tokenA/tokenB.
If UNI token would be used as a tokenA or tokenB, the transaction of the GMXVault#constructor() would be reverted due to that $UNI token can not approve with the amount, which is larger than uint96.

Vulnerability Details

Within the GMXVault#constructor(), the router and each Vault would be approved to spend the type(uint256).max amount of each tokenA/tokenB (_store.tokenA/_store.tokenB) like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L118-L119
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L122-L123
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L127-L128

/**
* @notice Initialize and configure vault's store, token approvals and whitelists
* @param name Name of vault
* @param symbol Symbol for vault token
* @param store_ GMXTypes.Store
*/
constructor (
string memory name,
string memory symbol,
GMXTypes.Store memory store_
) ERC20(name, symbol) Ownable(msg.sender) {
...
_store.tokenA = IERC20(store_.tokenA);
_store.tokenB = IERC20(store_.tokenB);
_store.lpToken = IERC20(store_.lpToken);
_store.WNT = IWNT(store_.WNT);
...
​
// Set token approvals for this vault
_store.tokenA.approve(address(_store.router), type(uint256).max); ///<----------- @audit
_store.tokenB.approve(address(_store.router), type(uint256).max); ///<----------- @audit
_store.lpToken.approve(address(_store.router), type(uint256).max);
​
_store.tokenA.approve(address(_store.depositVault), type(uint256).max); ///<----------- @audit
_store.tokenB.approve(address(_store.depositVault), type(uint256).max); ///<----------- @audit
​
_store.lpToken.approve(address(_store.withdrawalVault), type(uint256).max);
​
_store.tokenA.approve(address(_store.tokenALendingVault), type(uint256).max); ///<----------- @audit
_store.tokenB.approve(address(_store.tokenBLendingVault), type(uint256).max); ///<----------- @audit
...
}

According to the compatibilities in the README, $UNI token would be used in the SteadeFi protocol on Arbitrum like this:

Tokens on Arbitrum:
...

  • UNI: 0xfa7f8980b0f1e64a2062791cc3b0871572f1f7f0
    ...

  • UNI-USDC GM: 0xc7Abb2C5f3BF3CEB389dF0Eecd6120D451170B50

According to the Revert on Large Approvals & Transfers, UNI token can not be approved with the amount, which is larger than uint96 like this:

Some tokens (e.g. UNI, COMP) revert if the value passed to approve or transfer is larger than uint96.

However, within the GMXVault#constructor(), each Vault would be approved to spend the type(uint256).max amount of any tokenA/tokenB.
If UNI token would be used as a tokenA or tokenB, the transaction of the GMXVault#constructor() would be reverted due to that $UNI token can not approve with the amount, which is larger than uint96.

Note
The GMXTrove#constructor() has the same vulnerability like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXTrove.sol#L38-L39

Impact

If UNI token would be used as a tokenA or tokenB, the transaction of the GMXVault#constructor() would be reverted.

Tools Used

  • Foundry

Recommendations

Within the GMXVault#constructor(), consider adding a conditional branch in order to switch how much amount of tokenA/tokenB would be approved depends on the token address like this:

constructor (
string memory name,
string memory symbol,
GMXTypes.Store memory store_,
+ address uniToken
) ERC20(name, symbol) Ownable(msg.sender) {
...
_store.tokenA = IERC20(store_.tokenA);
_store.tokenB = IERC20(store_.tokenB);
_store.lpToken = IERC20(store_.lpToken);
_store.WNT = IWNT(store_.WNT);
...
​
// Set token approvals for this vault
+ If (tokenA == uniToken) {
+ _store.tokenA.approve(address(_store.router), type(uint96).max);
+ } else {
+ _store.tokenA.approve(address(_store.router), type(uint256).max);
+ }
+ If (tokenB == uniToken) {
+ _store.tokenB.approve(address(_store.router), type(uint96).max);
+ } else {
+ _store.tokenB.approve(address(_store.router), type(uint256).max);
+ }
- _store.tokenA.approve(address(_store.router), type(uint256).max);
- _store.tokenB.approve(address(_store.router), type(uint256).max);
_store.lpToken.approve(address(_store.router), type(uint256).max);
​
+ If (tokenA == uniToken) {
+ _store.tokenA.approve(address(_store.depositVault), type(uint96).max);
+ } else {
+ _store.tokenA.approve(address(_store.depositVault), type(uint256).max);
+ }
+ If (tokenB == uniToken) {
+ _store.tokenB.approve(address(_store.depositVault), type(uint96).max);
+ } else {
+ _store.tokenB.approve(address(_store.depositVault), type(uint256).max);
+ }
- _store.tokenA.approve(address(_store.depositVault), type(uint256).max);
- _store.tokenB.approve(address(_store.depositVault), type(uint256).max);
​
_store.lpToken.approve(address(_store.withdrawalVault), type(uint256).max);
​
+ If (tokenA == uniToken) {
+ _store.tokenA.approve(address(_store.tokenALendingVault), type(uint96).max);
+ } else {
+ _store.tokenA.approve(address(_store.tokenALendingVault), type(uint256).max);
+ }
+ If (tokenB == uniToken) {
+ _store.tokenB.approve(address(_store.tokenBLendingVault), type(uint96).max);
+ } else {
+ _store.tokenB.approve(address(_store.tokenBLendingVault), type(uint256).max);
+ }
- _store.tokenA.approve(address(_store.tokenALendingVault), type(uint256).max);
- _store.tokenB.approve(address(_store.tokenBLendingVault), type(uint256).max);
...
}

Also, within the GMXVault#constructor(), consider applying the same way with above.

Updates

Lead Judging Commences

hans Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.