The Buffer V2.5 repo was reviewed at hash 84b6060b44
In-Scope Contracts
- contracts/core/AccountRegistrar.sol
- contracts/core/Booster.sol
- contracts/core/BufferBinaryOptions.sol
- contracts/core/BufferBinaryPool.sol
- contracts/core/BufferRouter.sol
- contracts/core/CreationWindow.sol
- contracts/core/OptionMath.sol
- contracts/core/OptionConfig.sol
- contracts/core/Validator.sol
Deployment Chain(s)
- Arbitrum One Mainnet
Identifier | Title | Severity | Mitigated |
---|---|---|---|
[H-01] | Payments for coupons to Booster are irretrievable | High | ✔️ |
[H-02] | BufferBinaryPool can permanently lock funds on early exercise | High | ✔️ |
[H-03] | Market direction signature can be abused if privateKeeperMode is disabled | High | ✔️ |
[H-04] | closeAnytime timestamp is never validated against current timestamp | High | ✔️ |
[H-05] | closeAnyTime timestamp is never validated against pricing timestamp | High | ✔️ |
[M-01] | booster#buy fails to apply discount | Medium | ✔️ |
[M-02] | Transferred options can only be closed early by previous owner | Medium | ✔️ |
[L-01] | Users can buy coupons for options that don't exist | Low | ✔️ |
[L-02] | registerAccount sub-call in BufferRouter#openTrades can revert entire transaction | Low | ✔️ |
[I-01] | Booster#buy should support buying multiple boosts at a time | Info | ✔️ |
[I-02] | OptionMath#_decay is never used | Info | ✔️ |
uint256 price = couponPrice - discount;
require(token.balanceOf(user) >= price, "Not enough balance");
token.safeTransferFrom(user, address(this), couponPrice); <- @audit tokens transferred to this
userBoostTrades[tokenAddress][user]
.totalBoostTrades += MAX_TRADES_PER_BOOST;
emit BuyCoupon(tokenAddress, user, couponPrice);
Booster#buy transfers tokens to itself but has no way to recover these tokens, causing them to be permanently trapped in this contract.
Transfer fees to a configurable address such as admin
Mitigated here by sending fees to admin rather than the booster contract
BufferBinaryOptions.sol#L380-L395
if (option.expiration > closingTime) {
profit =
(option.lockedAmount *
OptionMath.blackScholesPriceBinary(
config.iv(),
option.strike,
closingPrice,
option.expiration - closingTime,
true,
isAbove
)) /
1e8;
} else {
profit = option.lockedAmount;
}
pool.send(optionID, user, profit);
When exercising an option early, it is possible to receive a partial payout depending on the factors such as when the contract is unlocked and the current asset price. This will cause the contract to send only a portion of the locked amount.
BufferBinaryPool.sol#L200-L207
uint256 transferTokenXAmount = tokenXAmount > ll.amount
? ll.amount
: tokenXAmount;
ll.locked = false;
lockedPremium = lockedPremium - ll.premium;
lockedAmount = lockedAmount - transferTokenXAmount;
tokenX.safeTransfer(to, transferTokenXAmount);
This is problematic since the amount of tokens sent is also the amount unlocked. This will leave the remainder of the fees perpetually locked in the BufferBinaryPool contract.
Example: A user opens an option that locks 100 USDC. After some time they decide to exercise early. Due to current pricing factors their option is exercised for only 50 USDC. This unlocks 50 USDC leaving the other 50 USDC permanently locked, which means they can never be withdrawn by LPs
BufferBinaryPool.sol#L191-L212
BufferBinaryPool#send should always unlock the entire option amount not just the amount sent.
Mitigated here. Upon exercise option.lockedAmount
is redeemed to the option contract. profit
is sent to the user and the remainder (if any) is sent back to the pool.
Buffer-Protocol-v2_5/contracts/core/BufferRouter.sol
Lines 233 to 246 in 84b6060
if (
!Validator.verifyMarketDirection(
params,
queuedTrade,
optionInfo.signer
)
) {
emit FailUnlock(
params.optionId,
params.targetContract,
"Router: Wrong market direction"
);
continue;
}
When options are exercised, the market direction is revealed via a signature provided by the opener. This can cause 2 significant issues if privateKeeperMode is disabled:
- User can change the direction of their trade after to guarantee they win
- User can open trade and withhold direction signature to indefinitely lock LP funds
Instead of using a signature at exercise, consider instead concatenating the direction with a salt then hashing storing that hash with the queued trade. Upon closure the salt and direction can be provided and the hash checked against the stored hash.
Mitigated here by removing the ability to disable privateKeeperMode
try
optionsContract.unlock(
params.optionId,
params.closingPrice,
publisherSignInfo.timestamp,
params.isAbove
)
{} catch Error(string memory reason) {
emit FailUnlock(params.optionId, params.targetContract, reason);
continue;
}
When exercising early, the timestamp used for unlocking is extremely important. The current implementation doesn't ever validate the current timestamp is anywhere near the current timestamp. If private keeper mode is disabled, a user could exercise their option in the past when they would get a better payoff.
Validate that exercise timestamp is within some margin of the current timestamp
Mitigated here by removing the ability to disable privateKeeperMode. This has only been addressed for non-trusted actors and can still be exploited by a malicious/compromised keeper
try
optionsContract.unlock(
params.optionId,
params.closingPrice,
publisherSignInfo.timestamp,
params.isAbove
)
{} catch Error(string memory reason) {
emit FailUnlock(params.optionId, params.targetContract, reason);
continue;
}
When an option is exercised early, it uses the timestamp of the pricing data without ever checking the timestamp of the closeAnytime signature. This can lead to 2 potential issues:
- The user may receive less than expected due to the actual exercise timestamp being different than when they signed
- If private keeper mode is disabled then transactions may be intercepted and the signature used by someone else to close the position with a much different timestamp
Pricing data timestamp and closeAnytime timestamp should be required to match
Mitigated here by removing the ability to disable privateKeeperMode. This has only been addressed for non-trusted actors and can still be exploited by a malicious/compromised keeper
uint256 price = couponPrice - discount;
re[Title](command:workbench.action.addRootFolder)quire(token.balanceOf(user) >= price, "Not enough balance");
token.safeTransferFrom(user, address(this), couponPrice); <- @audit transfers couponPrice rather than price
When buying boosts the contract mistakenly transfers couponPrice rather than price which is reduced by discount. Additionally the event also emits this incorrect value.
Transfer price
rather than couponPrice
Mitigated here by transferring price
rather than couponPrice
.
(address signer, ) = getAccountMapping(queuedTrade.user);
bool isUserSignValid = Validator.verifyCloseAnytime(
optionsContract.assetPair(),
closeParam.userSignInfo.timestamp,
params.optionId,
closeParam.userSignInfo.signature,
signer
);
Options are minted as NFTs allowing options to be transferred to other users. This allows users to potentially sell/transfer their options to other user. This cause 2 issues:
- The previous owner can close the option early to cause damage to the new owner
- The new owner cannot close their option early by themselves
NFT functionality should be removed or closeAnytime should determine the signer based on the owner of the option NFT
Mitigated here by only allowing positions to be transferred to/from approved addresses.
Booster#buy allows users to buy boosts for any tokens, which means that user can pay for and buy useless boost for tokens that aren't listed.
Consider limiting boost purchases to only tokens currently listed for better UX
Mitigated here the buy function was refactored to be callable only by owner and use permits for approval which negates the impact of this.
if (params[index].register.shouldRegister) {
accountRegistrar.registerAccount(
params[index].register.oneCT,
user,
params[index].register.signature
);
}
The openTrades function has been designed to prevent it from reverting under almost every circumstance. The exception to this is that accountRegistrar.registerAccount can revert the entire transaction.
Consider using a try-catch block to prevent it from reverting
Mitigated here by implementing a try-catch block
Booster#buy only allows the purchase of one boost at a time. Consider allowing users to bulk buy boosts to save transactions and gas.
Consider allowing multiple boosts to be purchased in a single transaction
Mitigated here by allowing the user to buy multiple coupons in a single transaction
OptionMath#_decay is never used and should be removed
OptionMath#_decay should be removed
Mitigated here by removing OptionMath#_decay