[WP-H10] A malicious early user/attacker can manipulate the pps to freeze users' funds at a certain deposit amount

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L476-L481

uint256 tokenId = depositors.mint(
    _msgSender(),
    amount,
    claimerId,
    _lockedUntil
);

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/node_modules/@openzeppelin/contracts/token/ERC721/ERC721.sol#L256-L266

function _safeMint(
    address to,
    uint256 tokenId,
    bytes memory _data
) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, _data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}

An early malicious user can send 1 wei of the vault token to the contract (as its wallet balance) then call deposit() as the first depositor of the Vault.

Beacuse deposit() -> depositors.mint() will mint a NFT to _msgSender() which can be an contract, this opens the oppotunity of reentrancy attck: the attacker can call deposit() again in onERC721Received().

As a result, the contract can malfunction or even freeze users' funds if the attack is planned sophisticatedly.

In the PoC bellow, we demoed an attack that essentially planted a bomb to the contract, which will be triggered once 1M are deposited and freeze users' funds for overflow due to the extremely large _totalShares.

PoC

1. Attacker send 1 wei of underlying token to the contract.
2. Attacker call deposit() with 340282366920938496 wei:

• newShares = 340282366920938496 * 10**18 (SHARES_MULTIPLIER)
• totalShares = newShares;

In onERC721Received(), Attacker call deposit() with 340282366920938496 wei again:

• newShares = (340282366920938496 340282366920938496 10**18) / 1
• totalShares = 115792089237316217907133152335680512000000000000000000;

💣 The bomb is now planted.

3. Bob call deposit() with 100,000 * 1e18 of underlying token:

• localTotalPrincipal = 340282366920938496 *2 + 1
• newShares = (100,000 1e18 115792089237316217907133152335680512000000000000000000) / localTotalPrincipal
• totalShares = 17014234138136162141217907133152335643777801536803518942350;

4. Charlie call deposit() with 1,000,000 * 1e18 of underlying token:

• localTotalPrincipal = 340282366920938496 2 + 1 + 100,000 1e18
• newShares = (1,000,000 1e18 17014234138136162141217907133152335643777801536803518942350) / localTotalPrincipal
• totalShares = 32481710148606587477395626066288346473515628339947249220056;

💥 The bomb is now triggered!

5. Charlie tries to call withdraw():

• localTotalShares = 32481710148606587477395626066288346473515628339947249220056
• localTotalPrincipal = 1100000680564733841876993;
• uint256 max = 115792089237316195423570985008687907853269984665640564039457584007913129639935;
• _amount * _totalShares = 187155417598605410391217907133152335276435816904838708365850000000000000000000000000;

The transaction will revert at _computeShares() due to overflow on _amount * _totalShares.

Recomandation

Consider using ReentrancyGuard from OpenZeppelin, add nonReentrant() modifiers to deposit() and other important entry functions.

[naps62]

duplicate of #3